Closed Bug 1399359 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox Build System :: Source Code Analysis, defect)

55 Branch
defect
Not set
normal

Tracking

(firefox57 fixed)

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: jya, Assigned: Sylvestre)

References

(Blocks 1 open bug, )

Details

Attachments

(1 file)

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
Blocks: clang-format
Assignee: nobody → sledru
./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 on attachment 8907431 [details]
Bug 1399359 - Enable clang-format option SplitEmptyFunction to have empty constructor correctly placed

https://reviewboard.mozilla.org/r/179118/#review184234
Attachment #8907431 - Flags: review?(jyavenard) → review+
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...
To have an occurrence of this? Myself, I see: 

-    bool HasPendingDrain() const
-    {
-      return mDrainState != DrainState::None;
-    }
+    bool HasPendingDrain() const { return mDrainState != DrainState::None; }
(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 on attachment 8907431 [details]
Bug 1399359 - Enable clang-format option SplitEmptyFunction to have empty constructor correctly placed

https://reviewboard.mozilla.org/r/179118/#review184240
Attachment #8907431 - Flags: review?(bpostelnicu) → review+
Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a13295273ef7
Enable clang-format option SplitEmptyFunction to have empty constructor correctly placed r=andi,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.
https://hg.mozilla.org/mozilla-central/rev/a13295273ef7
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
merged upstream too:
https://reviews.llvm.org/rL313182
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.