1036789 - Convert the third quarter of MFBT to Gecko style

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Nicholas Nethercote [inactive]]

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.

Comment 1

[Nicholas Nethercote [inactive]]

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.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: Indentation changes

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: Everything else

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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?

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 6

[Nicholas Nethercote [inactive]]

Thanks for the quick review!

Comment 7

[Nicholas Nethercote [inactive]]

> 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.

Comment 8

[Nicholas Nethercote [inactive]]

> 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).

Comment 9

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/91d7659bb351

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/91d7659bb351