1014377 - Convert the first quarter of MFBT to Gecko style

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Nicholas Nethercote [inactive]]

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

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (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.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 1b) - The rest

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.

Comment 3

[Nathan Froyd [:froydnj]]

(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/

Comment 4

[Nicholas Nethercote [inactive]]

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

[Nathan Froyd [:froydnj]]

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!

Comment 6

[Nathan Froyd [:froydnj]]

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.

Comment 7

[Nicholas Nethercote [inactive]]

> 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.

Comment 8

[Nicholas Nethercote [inactive]]

dev-platform agrees with me! So I left those one-liner functions unchanged.

https://hg.mozilla.org/integration/mozilla-inbound/rev/038794b1a5bc

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/038794b1a5bc