Closed Bug 1036789 Opened 10 years ago Closed 10 years ago

Convert the third quarter of MFBT to Gecko style

Categories

(Core :: MFBT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: n.nethercote, Assigned: n.nethercote)

References

Details

Attachments

(2 files)

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.
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.
Attachment #8453577 - Flags: review?(Ms2ger)
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Attached patch Everything elseSplinter Review
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+
Thanks for the quick review!
> 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.
> 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).
https://hg.mozilla.org/mozilla-central/rev/91d7659bb351
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Blocks: 1041914
You need to log in before you can comment on or make changes to this bug.