1026319 - Convert the second quarter of MFBT to Gecko style

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Nicholas Nethercote [inactive]]

This bug will convert these files:

> Endian.h 
> EnumeratedArray.h
> EnumSet.h
> FloatingPoint.cpp
> FloatingPoint.h
> GuardObjects.h 
> HashFunctions.cpp
> HashFunctions.h
> IntegerPrintfMacros.h
> IntegerTypeTraits.h  
> Likely.h 
> LinkedList.h
> lz4.c
> lz4_encoder.h
> lz4.h
> MacroArgs.h
> MacroForEach.h 
> MathAlgorithms.h
> MathAlgorithms.h.orig
> Maybe.h
> MaybeOneOf.h
> MemoryChecking.h
> MemoryReporting.h
> Move.h
> moz.build
> MSIntTypes.h
> NullPtr.h
> NumericLimits.h
> PodOperations.h
> Poison.cpp 
> Poison.h

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Convert the second quarter of MFBT to Gecko style (whitespace)

As before, this is just whitespace changes. |hg qdiff -w| is empty.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: Convert the second quarter of MFBT to Gecko style (lz4 line endings)

The lz4 code is third-party so I left it alone, except I converted the DOS line
endings to Unix line endings. Again, |hg qdiff -w| is empty for this patch.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: Convert the second quarter of MFBT to Gecko style (the rest)

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 8441094 [details] [diff] [review]
Convert the second quarter of MFBT to Gecko style (lz4 line endings)

Review of attachment 8441094 [details] [diff] [review]:
-----------------------------------------------------------------

I think you mixed up some whitespace changes and some lz4 changes in this patch...

Are the lz4 whitespace changes really buying us anything, considering that we might have to re-import it at some point and then we'd have to either (a) redo the changes; or (more likely) (b) put up with DOS line endings again?

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 8441096 [details] [diff] [review]
Convert the second quarter of MFBT to Gecko style (the rest)

Review of attachment 8441096 [details] [diff] [review]:
-----------------------------------------------------------------

Did you miss HashFunctions.cpp?  I realized that I haven't seen it in any of these patches.

::: mfbt/BloomFilter.h
@@ -197,5 @@
>    if (MOZ_LIKELY(!full(slot1))) {
>      ++slot1;
>    }
> -  uint8_t& slot2 = secondSlot(aHash); {
> -  if (MOZ_LIKELY(!full(slot2)))

O.o

::: mfbt/Endian.h
@@ +302,1 @@
>        union {

Move this to the next line.

@@ +329,1 @@
>        union {

This too.

@@ +595,1 @@
>      union {

This too.

::: mfbt/EnumSet.h
@@ +169,4 @@
>      uint8_t count = 0;
>      for (uint32_t bitField = mBitField; bitField; bitField >>= 1) {
>        if (bitField & 1)
>          count++;

Brace this if.

::: mfbt/EnumeratedArray.h
@@ +59,1 @@
>        mArray[i] = aOther.mArray[i];

Brace this loop.

::: mfbt/GuardObjects.h
@@ +78,2 @@
>  
> +  ~GuardObjectNotifier() { *mStatementDone = true; }

Taking full advantage of the new rules, I see.

::: mfbt/IntegerTypeTraits.h
@@ +24,5 @@
>  template<size_t Size, bool Signedness>
>  struct StdintTypeForSizeAndSignedness;
>  
>  template<>
> +struct StdintTypeForSizeAndSignedness<1, true>  { typedef int8_t   Type; };

I guess this is a natural extension of the one-line function rule to structs...but I'm not a fan.

Please change to the usual struct style and/or go campaign on dev-platform.

::: mfbt/LinkedList.h
@@ +341,3 @@
>      T* ret = sentinel.getNext();
>      if (ret)
>        static_cast<LinkedListElement<T>*>(ret)->remove();

Brace this if.

@@ +353,3 @@
>      T* ret = sentinel.getPrevious();
>      if (ret)
>        static_cast<LinkedListElement<T>*>(ret)->remove();

Brace this if.

@@ +375,2 @@
>      while (popFirst())
>        continue;

Brace this loop.

@@ +387,3 @@
>      size_t n = 0;
>      for (const T* t = getFirst(); t; t = t->getNext())
> +      n += aMallocSizeOf(t);

Brace this loop.

@@ +421,1 @@
>      {

Move the brace to the previous line.

@@ +432,1 @@
>      {

And this one.

@@ +444,1 @@
>      {

And this one.

@@ +467,2 @@
>        if (elem == t)
>          return;

Brace this if.

::: mfbt/PodOperations.h
@@ +136,5 @@
>     */
> +  for (const volatile T* srcend = aSrc + aNElem;
> +       aSrc < srcend;
> +       aSrc++, aDst++)
> +  {

Brace to the previous line.

Comment 6

[Nicholas Nethercote [inactive]]

I'll revert the lz4 changes.

> Did you miss HashFunctions.cpp?

I did! Well spotted.

Comment 7

[Nicholas Nethercote [inactive]]

> Move the brace to the previous line.

I can't find anything about it in the style guide, but I think it's a standard thing to put the brace on the next line when the condition is multi-line?

Comment 8

[(no longer active)]

(In reply to Nicholas Nethercote [:njn] from comment #7)
> > Move the brace to the previous line.
> 
> I can't find anything about it in the style guide, but I think it's a
> standard thing to put the brace on the next line when the condition is
> multi-line?

I don't think so!

Comment 9

[Andrew McCreight (out of office until 8/21) [:mccr8]]

That's one of the many exceptional conditions in the SpiderMonkey style rules, not in Gecko. ;)

Comment 10

[Nicholas Nethercote [inactive]]

Ok, I've moved those braces up. I now remember that it's a more useful exception when the indent level is four, because the block body and the indented conditions line up, like so:

> if (foo &&
>     bar) {
>     loop_body();
> }

That's not a problem with two-space indents:

> if (foo &&
>     bar) {
>   loop_body();
> }

Comment 11

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf068fd95d3c

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/cf068fd95d3c