Closed
Bug 1378250
Opened 7 years ago
Closed 7 years ago
clang-format should align argument names in argument lists
Categories
(Developer Infrastructure :: Source Code Analysis, defect)
Developer Infrastructure
Source Code Analysis
Tracking
(Not tracked)
RESOLVED
WONTFIX
People
(Reporter: MatsPalmgren_bugz, Unassigned)
References
Details
Attachments
(1 file)
Recently someone made this change:
> -nsCSSFrameConstructor::ContentRemoved(nsIContent* aContainer,
> - nsIContent* aChild,
> - nsIContent* aOldNextSibling,
> - RemoveFlags aFlags,
> - bool* aDidReconstruct,
> +nsCSSFrameConstructor::ContentRemoved(nsIContent* aContainer,
> + nsIContent* aChild,
> + nsIContent* aOldNextSibling,
> + RemoveFlags aFlags,
> + bool* aDidReconstruct,
> nsIContent** aDestroyedFramesFor)
and justified that with "clang-format ¯\_( ͡° ͜ʖ ͡°)_/¯".
IMO, the above change is a regression in readability.
The above is a fairly mild example since the first four
names align naturally, but there are far worse examples.
Can we make clang-format align the parameter names, please?
Updated•7 years ago
|
Assignee: nobody → bpostelnicu
Updated•7 years ago
|
Assignee: bpostelnicu → nobody
Comment hidden (mozreview-request) |
Comment 2•7 years ago
|
||
mozreview-review |
Comment on attachment 8883525 [details] Bug 1378250 - clang-format: Align consecutive declarations https://reviewboard.mozilla.org/r/154450/#review159536
Attachment #8883525 -
Flags: review?(bpostelnicu) → review+
Pushed by sledru@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b14cbbf5256a clang-format: Align consecutive declarations r=andi
Comment 4•7 years ago
|
||
Ugh, I think this is fine for arguments, but just tried this, and this makes code inside a function written as: nsStyleChangeList currentChanges(StyleBackendType::Servo); DocumentStyleRootIterator iter(doc); bool anyStyleChanged = false; To be formatted as: nsStyleChangeList currentChanges(StyleBackendType::Servo); DocumentStyleRootIterator iter(doc); bool anyStyleChanged = false; Which IMO is unheard-of in our codebase, and is quite ugly personally.
Comment 5•7 years ago
|
||
Also code like: ServoStyleSet* styleSet = StyleSet(); nsIDocument* doc = PresContext()->Document(); bool animationOnly = aRestyleBehavior == TraversalRestyleBehavior::ForAnimationOnly; Becomes: ServoStyleSet* styleSet = StyleSet(); nsIDocument* doc = PresContext()->Document(); bool animationOnly = aRestyleBehavior == TraversalRestyleBehavior::ForAnimationOnly; Which is unreadable IMO.
Comment 6•7 years ago
|
||
Indeed, this isn't great :/ Tomcat, could you backout my change? sorry :/
Flags: needinfo?(cbook)
Comment 7•7 years ago
|
||
(In reply to Sylvestre Ledru [:sylvestre] from comment #6) > Indeed, this isn't great :/ > > Tomcat, could you backout my change? > sorry :/ np :) done in https://hg.mozilla.org/integration/autoland/rev/e1169c7413b7be542afa07053a1c81a5e293ffad
Flags: needinfo?(cbook)
Comment 8•7 years ago
|
||
Should we wontfix this? While I understand Mats' concern in comment 0, there does not appear to be consensus on this preference. I imagine it's in the minority, but I can ping dev-platform and the code style module owners. Personally I find the blame and burden I take for realigning things every time I add a new longer param (or shorten a longer param) to be problematic (there's a similar argument for prefixing member lists with ':' and ','). If the tool is doing it for me, the main argument against is diff/blame bloat which I still find problematic. Also what should we do in the case of multi-line arguments with multiple params? > foo(bar* q, foobar* r, > bazbar* s, int t); does that become: > foo(bar* q, foobar* r, > bazbar* s, int t); Or maybe that's not a valid issue if we have a rule of one argument per line; I don't see that stated anywhere, but I might be missing something.
Comment 9•7 years ago
|
||
Per further discussion on the dev-platform mailing list [1] I'm going to wontfix this. [1] https://groups.google.com/forum/#!topic/mozilla.dev.platform/De36Q5r9ZKo
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
Comment 10•7 years ago
|
||
FWIW AlignConsecutiveDeclarations wasn't designed to only work on declarations inside functions' argument lists, so the results we saw here were the intended results.
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
•