Sorry, I know, coding style thread... But it's Friday and this is 
somewhat related to the previous thread.

Bug 525063 added a lot of lines like:

     explicit TTextAttr(bool aGetRootValue)
       : mGetRootValue(aGetRootValue)
       , mIsDefined{ false }
       , mIsRootDefined{ false }
     {
     }

I think I find them hard to read and ugly.

Those changes I assume were generated with clang-format / 
clang-format-diff using the "Mozilla" coding style, so I'd rather ask 
people to agree in whether we prefer that style or other in order to 
change that if needed.

Would people agree to use:

  , mIsRootDefined { false }

Instead of:

  , mIsRootDefined{ false }

And:

  , mFoo { }

Instead of:

  , mFoo{}

?

I assume the same would be for variables, I find:

   AutoRestore<bool> restore { mFoo };

easier to read than:

   AutoRestore<bool> restore{ mFoo };

What's people's opinion on that? Would people be fine with a more 
general "spaces around braces" rule? I can't think of a case right now 
where I personally wouldn't prefer it.

Also, we should probably state that consistency is preferred (I assume 
we generally agree on that), so in this case braces probably weren't 
even needed, or everything should've switched to them.

Finally, while I'm here, regarding default member initialization, what's 
preferred?

   uint32_t* mFoo = nullptr;

Or:

   uint32_t* mFoo { nullptr };

I'm ambivalent, but brace syntax should cover more cases IIUC (that is, 
there are things that you can write with braces that you couldn't with 
equals I _think_).

Should we state a preference? Or just say that both are allowed but 
consistency is better?

  -- Emilio

On 4/13/18 9:37 AM, Emilio Cobos Álvarez wrote:
> Would people agree to use:
> 
>   , mIsRootDefined { false }
> 
> Instead of:
> 
>   , mIsRootDefined{ false }

So my take is that we should not use braced initializer syntax in 
constructor initializer lists.  The reason for that is that it makes it 
much harder to scan for where the constructor body starts.  Doubly so 
when ifdefs in the initializer list are involved.  Triply so when 
someone writes it as:

   explicit TTextAttr()
     , mIsRootDefined
   {
     false
   }
#ifdef SOMETHING
#endif
   {
   }

which is what clang-format did in some of the cases in the patch for bug 
525063.

In particular, I think this should have just used:

   , mIsRootDefined(false)

I don't have a strong opinion about the one space when we do use the 
braced initializer syntax.  But we should make sure we don't end up with 
the above monstrosity where it looks like the ctor body.

-Boris

On 4/13/18 4:22 PM, Boris Zbarsky wrote:
> So my take is that we should not use braced initializer syntax in 
> constructor initializer lists.  The reason for that is that it makes it 
> much harder to scan for where the constructor body starts.

I don't think that's true in the general case where the braces remain to 
the right of the identifier in the same line, fwiw, but...

> Doubly so when ifdefs in the initializer list are involved. > Triply so when someone writes it as:
> 
>    explicit TTextAttr()
>      , mIsRootDefined
>    {
>      false
>    }
> #ifdef SOMETHING
> #endif
>    {
>    }
> 
> which is what clang-format did in some of the cases in the patch for bug 
> 525063.

.... this is definitely terrible I agree :(

> In particular, I think this should have just used:
> 
>    , mIsRootDefined(false)

Agreed in this case (specially given there were previous members 
initialized with parenthesis, and the braces are not really necessary).

> I don't have a strong opinion about the one space when we do use the 
> braced initializer syntax.  But we should make sure we don't end up with 
> the above monstrosity where it looks like the ctor body.

I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1453973 for that.

> -Boris
> _______________________________________________
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform