If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Convert the third quarter of MFBT to Gecko style

RESOLVED FIXED in mozilla33

Status

()

Core
MFBT
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla33
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

3 years ago
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

3 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

3 years ago
Created attachment 8453577 [details] [diff] [review]
Indentation changes
Attachment #8453577 - Flags: review?(Ms2ger)
(Assignee)

Updated

3 years ago
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
(Assignee)

Comment 3

3 years ago
Created attachment 8453579 [details] [diff] [review]
Everything else
Attachment #8453579 - Flags: review?(Ms2ger)
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 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

3 years ago
Thanks for the quick review!
(Assignee)

Comment 7

3 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

3 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

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/91d7659bb351
https://hg.mozilla.org/mozilla-central/rev/91d7659bb351
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
(Assignee)

Updated

3 years ago
Blocks: 1041914
You need to log in before you can comment on or make changes to this bug.