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)
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
| Assignee | ||
Updated•4 years ago
|
Blocks: clang-format
| Assignee | ||
Updated•4 years ago
|
Assignee: nobody → sledru
| Comment hidden (mozreview-request) |
| Assignee | ||
Comment 2•4 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•4 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•4 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•4 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•4 years ago
|
||
Reported upstream: https://reviews.llvm.org/D37795
Comment 7•4 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•4 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•4 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•4 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/a13295273ef7
Status: NEW → RESOLVED
Closed: 4 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
| Assignee | ||
Comment 12•4 years ago
|
||
merged upstream too: https://reviews.llvm.org/rL313182
Updated•3 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•