Closed
Bug 1026319
Opened 10 years ago
Closed 10 years ago
Convert the second quarter of MFBT to Gecko style
Categories
(Core :: MFBT, defect)
Core
MFBT
Tracking
()
RESOLVED
FIXED
mozilla33
People
(Reporter: n.nethercote, Assigned: n.nethercote)
Details
Attachments
(3 files)
47.80 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
126.84 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
129.40 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•10 years ago
|
||
As before, this is just whitespace changes. |hg qdiff -w| is empty.
Attachment #8441093 -
Flags: review?(nfroyd)
Assignee | ||
Comment 2•10 years ago
|
||
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.
Attachment #8441094 -
Flags: review?(nfroyd)
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8441096 -
Flags: review?(nfroyd)
Updated•10 years ago
|
Attachment #8441093 -
Flags: review?(nfroyd) → review+
Comment 4•10 years ago
|
||
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?
Attachment #8441094 -
Flags: review?(nfroyd) → review+
Comment 5•10 years ago
|
||
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.
Attachment #8441096 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 6•10 years ago
|
||
I'll revert the lz4 changes.
> Did you miss HashFunctions.cpp?
I did! Well spotted.
Assignee | ||
Comment 7•10 years ago
|
||
> 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•10 years ago
|
||
(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•10 years ago
|
||
That's one of the many exceptional conditions in the SpiderMonkey style rules, not in Gecko. ;)
Assignee | ||
Comment 10•10 years ago
|
||
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(); > }
Assignee | ||
Comment 11•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf068fd95d3c
Comment 12•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/cf068fd95d3c
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
•