Closed
Bug 1014377
Opened 10 years ago
Closed 10 years ago
Convert the first quarter of MFBT to Gecko style
Categories
(Core :: MFBT, defect)
Core
MFBT
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: n.nethercote, Assigned: n.nethercote)
Details
Attachments
(2 files)
99.52 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
96.91 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
This bug will convert these files:
> Alignment.h
> AllocPolicy.h
> Array.h
> ArrayUtils.h
> Assertions.h
> Atomics.h
> Attributes.h
> BinarySearch.h
> BloomFilter.h
> Casting.h
> ChaosMode.h
> Char16.h
> CheckedInt.h
> Compiler.h
> Compression.cpp
> Compression.h
> Constants.h
> DebugOnly.h
Assignee | ||
Comment 1•10 years ago
|
||
|hg qdiff -w| is empty for this patch.
There's one case I didn't change -- MFBT indents preprocessor code within
#ifdefs, like this:
> #ifdef FOO
> # include <foo.h>
> #endif
This is not how Gecko typically does it, and I thought about just removing all
such indentation... but there are some files like Attributes.h that are
jam-packed with such code, so I left it alone. I did change some indenting of
non-preprocessor code within #ifdefs, because indenting in that case is highly
non-standard.
Attachment #8426806 -
Flags: review?(nfroyd)
Assignee | ||
Comment 2•10 years ago
|
||
Notes:
- An agreement I reached with Waldo a while back was to not insist on CamelCase
for functions and methods within MFBT. The rationale was that this would keep
these style changes within mfbt/.
For the same reason, I didn't rename DebugOnly::value as mValue, though I
could (and introduce a value() getter) if you think it's worthwhile.
- Also, I'm not sure if having class methods on a single line like this:
> int foo(int aValue) { return -aValue; }
is kosher by Gecko guidelines, but I think it's reasonable for very short
functions and I've left function like that untouched in most cases.
I'll merge this with part 1a before landing to minimize |hg blame| upheaval.
Feel free to take some time on this; I know you just reviewed a bunch of xpcom
style fixes in another bug.
Attachment #8426815 -
Flags: review?(nfroyd)
Comment 3•10 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #1) > Created attachment 8426806 [details] [diff] [review] > (part 1a) - Indentation changes > > |hg qdiff -w| is empty for this patch. > > There's one case I didn't change -- MFBT indents preprocessor code within > #ifdefs, like this: > > > #ifdef FOO > > # include <foo.h> > > #endif > > This is not how Gecko typically does it, and I thought about just removing > all > such indentation... but there are some files like Attributes.h that are > jam-packed with such code, so I left it alone. I did change some indenting > of > non-preprocessor code within #ifdefs, because indenting in that case is > highly > non-standard. I kind of like the MFBT style regarding #directives, personally. (Though, in some ways, if you have to indent your #ifdefs to understand what's going on, you're probably Doing Something Wrong.) (In reply to Nicholas Nethercote [:njn] from comment #2) > - An agreement I reached with Waldo a while back was to not insist on > CamelCase > for functions and methods within MFBT. The rationale was that this would > keep > these style changes within mfbt/. That's fine. For some of the template metaprogramming stuff, it's probably better that we maintain notional consistency with the STL anyway. > For the same reason, I didn't rename DebugOnly::value as mValue, though I > could (and introduce a value() getter) if you think it's worthwhile. I am guessing that DebugOnly::value is used comparatively rarely, so this is OK. > - Also, I'm not sure if having class methods on a single line like this: > > > int foo(int aValue) { return -aValue; } > > is kosher by Gecko guidelines, but I think it's reasonable for very short > functions and I've left function like that untouched in most cases. The XPCOM style patches moved these onto multiple lines. I think we had a dev-platform discussion about this, but I don't remember what the outcome was. I am ambivalent, as I see arguments both ways, but consistency is probably better. Breaking these can be done in a separate patch, unless one or both of us drags out the dev-platform thread and attempts to extract a consensus (or starts a new thread...) > I'll merge this with part 1a before landing to minimize |hg blame| upheaval. \o/
Assignee | ||
Comment 4•10 years ago
|
||
I find some all-on-one-line cases much easier to read. E.g. catching typos in this: > static ValueType inc(ValueType& ptr) { return add(ptr, 1); } > static ValueType dec(ValueType& ptr) { return sub(ptr, 1); } is much easier than this: > static ValueType inc(ValueType& ptr) > { > return add(ptr, 1); > } > > static ValueType dec(ValueType& ptr) > { > return sub(ptr, 1); > }
Comment 5•10 years ago
|
||
Comment on attachment 8426806 [details] [diff] [review] (part 1a) - Indentation changes Review of attachment 8426806 [details] [diff] [review]: ----------------------------------------------------------------- ::: mfbt/Assertions.h @@ +36,4 @@ > # endif > + __declspec(dllimport) int __stdcall > + TerminateProcess(void* hProcess, unsigned int uExitCode); > + __declspec(dllimport) void* __stdcall GetCurrentProcess(void); These should be flush left; we'd normally not indent |extern "C"| blocks and we don't have special rules for code in #ifdef blocks. ::: mfbt/Attributes.h @@ +149,5 @@ > * > * template<typename T> > * class Ptr > * { > + * T* ptr; Going above and beyond to reindent example code, too!
Attachment #8426806 -
Flags: review?(nfroyd) → review+
Comment 6•10 years ago
|
||
Comment on attachment 8426815 [details] [diff] [review] (part 1b) - The rest Review of attachment 8426815 [details] [diff] [review]: ----------------------------------------------------------------- The style guide discussion: https://groups.google.com/d/msg/mozilla.dev.platform/0ixYMnOKBog/MYsLQS5LFe0J on the small/empty inline-in-classes method bodies decreed that Thou Shalt Put Thy Braces On Separate Lines. It wouldn't be a style guide if everybody liked the rules. ;) r=me with all the style nits below fixed. ::: mfbt/Array.h @@ +18,5 @@ > > template<typename T, size_t Length> > class Array > { > T arr[Length]; No renaming? Want to keep the pirate spirit alive? ::: mfbt/Atomics.h @@ +483,5 @@ > template<typename T> > struct IntrinsicIncDec : public IntrinsicAddSub<T> > { > + static T inc(T& aPtr) { return IntrinsicAddSub<T>::add(aPtr, 1); } > + static T dec(T& aPtr) { return IntrinsicAddSub<T>::sub(aPtr, 1); } Split these out onto separate lines? @@ +492,5 @@ > public IntrinsicIncDec<T> > { > + static T or_( T& aPtr, T aVal) { return __sync_fetch_and_or(&aPtr, aVal); } > + static T xor_(T& aPtr, T aVal) { return __sync_fetch_and_xor(&aPtr, aVal); } > + static T and_(T& aPtr, T aVal) { return __sync_fetch_and_and(&aPtr, aVal); } These too? @@ +752,5 @@ > template<typename PrimType, typename T> > struct CastHelper > { > + static PrimType toPrimType(T aVal) { return static_cast<PrimType>(aVal); } > + static T fromPrimType(PrimType aVal) { return static_cast<T>(aVal); } Separate lines. @@ +759,5 @@ > template<typename PrimType, typename T> > struct CastHelper<PrimType, T*> > { > + static PrimType toPrimType(T* aVal) { return reinterpret_cast<PrimType>(aVal); } > + static T* fromPrimType(PrimType aVal) { return reinterpret_cast<T*>(aVal); } Separate lines. @@ +884,5 @@ > struct IntrinsicIncDec : public IntrinsicAddSub<T> > { > typedef typename IntrinsicAddSub<T>::ValueType ValueType; > + static ValueType inc(ValueType& aPtr) { return add(aPtr, 1); } > + static ValueType dec(ValueType& aPtr) { return sub(aPtr, 1); } Separate lines. ::: mfbt/BloomFilter.h @@ +147,5 @@ > > private: > + static const size_t sArraySize = (1 << KeySize); > + static const uint32_t sKeyMask = (1 << KeySize) - 1; > + static const uint32_t sKeyShift = 16; I think const-ness ought to trump static-ness in the naming convention, so these would be kArraySize, kKeyMask, and kKeyShift. ::: mfbt/Casting.h @@ +60,5 @@ > // Unsigned-to-unsigned range check > > template<typename From, typename To, > + UUComparison = (sizeof(From) > sizeof(To)) ? FromIsBigger > + : FromIsNotBigger> The style guide seems to want this as: ... = (sizeof(From) > sizeof(To)) ? FromIsBigger : FromIsNotBigger given that we want all non-&& and non-|| operators to begin manually-broken lines. Though I'm pretty sure I've gotten and/or seen review comments that want all binary operators at the end of lines. grepping through dom/ shows...well, all sorts of things, but the two main choices are: - break after the ?, and also break after the : - break before the ?, like so: ... = (sizeof(From) > sizeof(To)) ? FromIsBigger : FromIsNotBigger This last is my preference for such cases. WDYT? @@ +117,5 @@ > enum USComparison { FromIsSmaller, FromIsNotSmaller }; > > template<typename From, typename To, > + USComparison = (sizeof(From) < sizeof(To)) ? FromIsSmaller > + : FromIsNotSmaller> Same ternary operator comment/resolution applies here too. ::: mfbt/Char16.h @@ +68,3 @@ > > public: > + char16ptr_t(const char16_t* aPtr) : mPtr(aPtr) {} Separate lines. @@ +68,5 @@ > > public: > + char16ptr_t(const char16_t* aPtr) : mPtr(aPtr) {} > + char16ptr_t(const wchar_t* aPtr) : > + mPtr(reinterpret_cast<const char16_t*>(aPtr)) Regardless of line length concerns here, I think the guide says: char16ptr_t(...) : mPtr(...) is the way to do this. @@ +69,5 @@ > public: > + char16ptr_t(const char16_t* aPtr) : mPtr(aPtr) {} > + char16ptr_t(const wchar_t* aPtr) : > + mPtr(reinterpret_cast<const char16_t*>(aPtr)) > + {} Separate lines. @@ +74,3 @@ > > /* Without this, nullptr assignment would be ambiguous. */ > + constexpr char16ptr_t(decltype(nullptr)) : mPtr(nullptr) {} Separate lines. ::: mfbt/DebugOnly.h @@ +40,5 @@ > > DebugOnly() { } > + DebugOnly(const T& aOther) : value(aOther) { } > + DebugOnly(const DebugOnly& aOther) : value(aOther.value) { } > + DebugOnly& operator=(const T& aRhs) { Separate lines for all of these. @@ +46,5 @@ > return *this; > } > + > + void operator++(int) { value++; } > + void operator--(int) { value--; } ...and these. @@ +51,2 @@ > > T* operator&() { return &value; } ...and all the remaining bits of the methods, too.
Attachment #8426815 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 7•10 years ago
|
||
> The style guide discussion: > > https://groups.google.com/d/msg/mozilla.dev.platform/0ixYMnOKBog/MYsLQS5LFe0J > > on the small/empty inline-in-classes method bodies decreed that Thou Shalt > Put Thy Braces On Separate Lines. It wouldn't be a style guide if everybody > liked the rules. ;) I emailed dev-platform to re-open that debate. We'll see how that pans out. I'll wait until it resolves before landing the patch. > ... = (sizeof(From) > sizeof(To)) > ? FromIsBigger > : FromIsNotBigger > > This last is my preference for such cases. WDYT? Yeah, that's good.
Assignee | ||
Comment 8•10 years ago
|
||
dev-platform agrees with me! So I left those one-liner functions unchanged. https://hg.mozilla.org/integration/mozilla-inbound/rev/038794b1a5bc
Comment 9•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/038794b1a5bc
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in
before you can comment on or make changes to this bug.
Description
•