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
![]() |
0 |
![]() |
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
![]() |
0 |
![]() |
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
![]() |
0 |
![]() |