1399359 - [clang-format] Empty constructors brackets not correctly placed.

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[Jean-Yves Avenard [:jya]]

This is related to bug 1340083

  class foo
  {
    Foo(int a, int, b, int c, int d)
      : mA(a)
      , mB(b)
      , mC(c)
      , mD(d)
    {
    }

    int a;
    int b;
    int c;
    int d;
  };

gets rewritten into:
  class foo
  {
    Foo(int a, int, b, int c, int d)
      : mA(a)
      , mB(b)
      , mC(c)
      , mD(d)
    {}

    int a;
    int b;
    int c;
    int d;
  };

it shouldn't, the original code is per style.

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Classes

the example that multiple lines constructor always have their braces on separate line.

Only single line constructor have their braces on the same line

Comment 1

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1399359 - Enable clang-format option SplitEmptyFunction to have empty constructor correctly placed

Review commit: https://reviewboard.mozilla.org/r/179118/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/179118/

Comment 2

[Sylvestre Ledru [:Sylvestre]]

./mach clang-format -p dom/media/MediaFormatReader.h
Show that we keep the formatting.

I will follow up upstream to see our coding style update in the product.

Comment 3

[Jean-Yves Avenard [:jya]]

Comment on attachment 8907431 [details]
Bug 1399359 - Enable clang-format option SplitEmptyFunction to have empty constructor correctly placed

https://reviewboard.mozilla.org/r/179118/#review184234

Comment 4

[Jean-Yves Avenard [:jya]]

verified as working (tested with enum, struct, classes and so forth) ... one line constructors stay on one line

only thing I noticed is that small functions such as:
void foo() { }

are rewritten to:
void
foo()
{
}

but maybe that's good...

Comment 5

[Sylvestre Ledru [:Sylvestre]]

To have an occurrence of this? Myself, I see: 

-    bool HasPendingDrain() const
-    {
-      return mDrainState != DrainState::None;
-    }
+    bool HasPendingDrain() const { return mDrainState != DrainState::None; }

Comment 6

[Sylvestre Ledru [:Sylvestre]]

Reported upstream: https://reviews.llvm.org/D37795

Comment 7

[Andi [:andi]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #5)
> To have an occurrence of this? Myself, I see: 
> 
> -    bool HasPendingDrain() const
> -    {
> -      return mDrainState != DrainState::None;
> -    }
> +    bool HasPendingDrain() const { return mDrainState != DrainState::None; }
Personally I see this as being normal and in line with the rest of the logic for member functions/ constructors.

(In reply to Jean-Yves Avenard [:jya] from comment #4)
> verified as working (tested with enum, struct, classes and so forth) ... one
> line constructors stay on one line
> 
> only thing I noticed is that small functions such as:
> void foo() { }
> 
> are rewritten to:
> void
> foo()
> {
> }
> 
> but maybe that's good...
I also see this ok as it's not a member function. Hope I'm not mistaking though.

Comment 8

[Andi [:andi]]

Comment on attachment 8907431 [details]
Bug 1399359 - Enable clang-format option SplitEmptyFunction to have empty constructor correctly placed

https://reviewboard.mozilla.org/r/179118/#review184240

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a13295273ef7
Enable clang-format option SplitEmptyFunction to have empty constructor correctly placed r=andi,jya

Comment 10

[Jean-Yves Avenard [:jya]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #5)
> To have an occurrence of this? Myself, I see: 
> 
> -    bool HasPendingDrain() const
> -    {
> -      return mDrainState != DrainState::None;
> -    }
> +    bool HasPendingDrain() const { return mDrainState != DrainState::None; }

well, it does fit in 80 columns...

so I think that's okay here.

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/a13295273ef7

Comment 12

[Sylvestre Ledru [:Sylvestre]]

merged upstream too:
https://reviews.llvm.org/rL313182