Closed
Bug 1036789
Opened 10 years ago
Closed 10 years ago
Convert the third quarter of MFBT to Gecko style
Categories
(Core :: MFBT, defect)
Core
MFBT
Tracking
()
RESOLVED
FIXED
mozilla33
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
Attachments
(2 files)
111.02 KB,
patch
|
Ms2ger
:
review+
|
Details | Diff | Splinter Review |
143.29 KB,
patch
|
Ms2ger
:
review+
|
Details | Diff | Splinter Review |
In this bug I will convert files mfbt/RangedPtr.h .. mfbt/WindowsVersion.h to Gecko style, continuing the work started in bug 1014377 and bug 1026319.
Assignee | ||
Comment 1•10 years ago
|
||
Ms2ger, froydnj has gone on vacation for two weeks so you've drawn the short straw for reviewing this bug. Sorry. The following two patches convert the files mfbt/RangedPtr.h .. mfbt/WindowsVersion.h to Gecko style. The first patch just fixes indentation, and |hg qdiff -w| is empty. The second patch fixes everything else. I'll fold them together before landing.
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8453577 -
Flags: review?(Ms2ger)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8453579 -
Flags: review?(Ms2ger)
Comment 4•10 years ago
|
||
Comment on attachment 8453579 [details] [diff] [review] Everything else Review of attachment 8453579 [details] [diff] [review]: ----------------------------------------------------------------- > Bug 1036789 - Everything else. Better commit message, please. I assume we're not changing methods to avoid changing all the callers? r=me with changes below. ::: mfbt/RangedPtr.h @@ +60,4 @@ > } > > + /* Creates a new pointer for |aPtr|, restricted to this pointer's range. */ > + RangedPtr<T> create(T *aPtr) const * to the left @@ +69,4 @@ > #endif > } > > + uintptr_t asUintptr() const { return uintptr_t(mPtr); } Make this a static_cast while you're here @@ +88,5 @@ > #endif > { > + MOZ_ASSERT(aLength <= size_t(-1) / sizeof(T)); > + MOZ_ASSERT(uintptr_t(mRangeStart) + aLength * sizeof(T) >= > + uintptr_t(mRangeStart)); Ditto @@ +101,5 @@ > #endif > { > + MOZ_ASSERT(aLength <= size_t(-1) / sizeof(T)); > + MOZ_ASSERT(uintptr_t(mRangeStart) + aLength * sizeof(T) >= > + uintptr_t(mRangeStart)); Ditto ::: mfbt/RefPtr.h @@ +96,5 @@ > { > friend class RefPtr<T>; > > protected: > + RefCounted() : refCnt(0) {} mRefCnt? @@ +265,5 @@ > > + TemporaryRef<T> forget() > + { > + T* tmp = mPtr; > + mPtr = 0; nullptr @@ +327,5 @@ > > + T* drop() const > + { > + T* tmp = mPtr; > + mPtr = 0; Ditto @@ +392,5 @@ > } // namespace mozilla > > #if 0 > > // Command line that builds these tests I assume you built them? @@ +411,4 @@ > } > > + bool mDead; > + static int mNumDestroyed; sFoo for statics ::: mfbt/SHA1.cpp @@ +207,5 @@ > * results in code that is 3 times faster than the previous NSS sha_fast > * code on AMD64. > */ > static void > +shaCompress(volatile unsigned *aX, const uint32_t *aBuf) * ::: mfbt/SHA1.h @@ +37,5 @@ > class SHA1Sum > { > + union > + { > + uint32_t mW[16]; /* input buffer */ 2-spacce ::: mfbt/TypedEnumBits.h @@ +4,5 @@ > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > +/* > + * MOZ_MAKE_ENUM_CLASS_BITWISE_OPERATORS allows using a typed enum as bit flags. */ revert ::: mfbt/Vector.h @@ +584,2 @@ > */ > + void erase(T* aBegin, T *aEnd); * @@ +1264,5 @@ > > public: > explicit Vector(AllocPolicy alloc = AllocPolicy()) : Base(alloc) {} > Vector(Vector&& vec) : Base(Move(vec)) {} > + Vector& operator=(Vector&& aOther) { Move the { ::: mfbt/WeakPtr.h @@ +131,5 @@ > template <typename T, class WeakReference> > class SupportsWeakPtrBase > { > public: > WeakPtrBase<T, WeakReference> asWeakPtr() { { @@ +139,5 @@ > return WeakPtrBase<T, WeakReference>(weakRef); > } > > protected: > ~SupportsWeakPtrBase() { { ::: mfbt/tests/TestSHA1.cpp @@ +201,1 @@ > MOZ_RELEASE_ASSERT(hash[i] == expected[i]); {} ::: netwerk/cache2/CacheFileIOManager.h @@ +114,5 @@ > > HandleHashKey(KeyTypePointer aKey) > { > MOZ_COUNT_CTOR(HandleHashKey); > + mHash = (SHA1Sum::Hash*)new uint8_t[SHA1Sum::kHashSize]; Do we need the cast?
Attachment #8453579 -
Flags: review?(Ms2ger) → review+
Comment 5•10 years ago
|
||
Comment on attachment 8453577 [details] [diff] [review] Indentation changes Review of attachment 8453577 [details] [diff] [review]: ----------------------------------------------------------------- Bug 1036789 - Indentation changes. Better commit message. ::: mfbt/SHA1.h @@ +37,5 @@ > class SHA1Sum > { > + union { > + uint32_t w[16]; /* input buffer */ > + uint8_t b[64]; 2 fewer spaces
Attachment #8453577 -
Flags: review?(Ms2ger) → review+
Assignee | ||
Comment 6•10 years ago
|
||
Thanks for the quick review!
Assignee | ||
Comment 7•10 years ago
|
||
> I assume we're not changing methods to avoid changing all the callers?
Changing the first letter to upper-case? Yeah. That's the one difference with standard Gecko style. Waldo suggested leaving those unchanged to avoid churn.
Assignee | ||
Comment 8•10 years ago
|
||
> Make this a static_cast while you're here They all required reinterpret_cast, so I did that instead. > I assume you built them? I didn't, but I have now. > > + * MOZ_MAKE_ENUM_CLASS_BITWISE_OPERATORS allows using a typed enum as bit flags. */ > > revert It's more than 80 chars. I slightly botched the change; I've fixed it properly now. > ::: netwerk/cache2/CacheFileIOManager.h > @@ +114,5 @@ > > > > HandleHashKey(KeyTypePointer aKey) > > { > > MOZ_COUNT_CTOR(HandleHashKey); > > + mHash = (SHA1Sum::Hash*)new uint8_t[SHA1Sum::kHashSize]; > > Do we need the cast? Apparently so: > 0:07.00 ../../../netwerk/cache2/CacheFileIOManager.h:118:13: error: no viable overloaded '=' > 0:07.00 mHash = new uint8_t[SHA1Sum::kHashSize]; > 0:07.00 ~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > 0:07.00 ../../dist/include/nsAutoPtr.h:490:3: note: candidate function not viable: no known conversion from 'uint8_t *' (aka 'unsigned char *') to 'unsigned char *[20]' for 1st argument > 0:07.00 operator=(T* aRhs) > 0:07.00 ^ > 0:07.00 ../../dist/include/nsAutoPtr.h:497:22: note: candidate function not viable: no known conversion from 'uint8_t *' (aka 'unsigned char *') to 'nsAutoArrayPtr<unsigned char [20]> &' for 1st argument > 0:07.00 nsAutoArrayPtr<T>& operator=(nsAutoArrayPtr<T>& aRhs) I addressed all the other comments, and also fixed a few more cases where '*' and '&' were on the right rather than the left (found with grep).
Assignee | ||
Comment 9•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/91d7659bb351
Comment 10•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/91d7659bb351
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in
before you can comment on or make changes to this bug.
Description
•