Closed
Bug 1399359
Opened 7 years ago
Closed 7 years ago
[clang-format] Empty constructors brackets not correctly placed.
Categories
(Developer Infrastructure :: Source Code Analysis, defect)
Tracking
(firefox57 fixed)
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: jya, Assigned: Sylvestre)
References
()
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
Assignee | ||
Updated•7 years ago
|
Blocks: clang-format
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → sledru
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•7 years ago
|
||
./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.
Reporter | ||
Comment 3•7 years ago
|
||
mozreview-review |
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+
Reporter | ||
Comment 4•7 years ago
|
||
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...
Assignee | ||
Comment 5•7 years ago
|
||
To have an occurrence of this? Myself, I see: - bool HasPendingDrain() const - { - return mDrainState != DrainState::None; - } + bool HasPendingDrain() const { return mDrainState != DrainState::None; }
Assignee | ||
Comment 6•7 years ago
|
||
Reported upstream: https://reviews.llvm.org/D37795
Comment 7•7 years ago
|
||
(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•7 years ago
|
||
mozreview-review |
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
Reporter | ||
Comment 10•7 years ago
|
||
(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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a13295273ef7
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Assignee | ||
Comment 12•7 years ago
|
||
merged upstream too: https://reviews.llvm.org/rL313182
Updated•6 years ago
|
Product: Core → Firefox Build System
Updated•2 years ago
|
Product: Firefox Build System → Developer Infrastructure
You need to log in
before you can comment on or make changes to this bug.
Description
•