Closed
Bug 1322321
Opened 8 years ago
Closed 8 years ago
Update the clang format file to match more our coding style
Categories
(Developer Infrastructure :: Source Code Analysis, defect)
Developer Infrastructure
Source Code Analysis
Tracking
(firefox53 fixed)
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: Sylvestre, Assigned: Sylvestre)
References
Details
Attachments
(1 obsolete file)
Mentioned in bug 1317821, we can do a better job in term of our coding style.
I will propose a patch to clang-format itself to see the Mozilla coding style updated.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → sledru
Assignee | ||
Comment 2•8 years ago
|
||
Change also proposed upstream:
https://reviews.llvm.org/D27557
Comment 3•8 years ago
|
||
mozreview-review |
Comment on attachment 8817111 [details]
Bug 1322321 - Update the clang format file to match more our coding style
https://reviewboard.mozilla.org/r/97540/#review98034
::: .clang-format:37
(Diff revision 1)
> +# All of the following items are default
> +# in the Mozila coding style from clang format 4.0
> +AlwaysBreakAfterReturnType: TopLevel
> +BinPackArguments: false
> +BinPackParameters: false
> +SpaceAfterTemplateKeyword: false
Don't you want true for this one?
Attachment #8817111 -
Flags: review?(ehsan) → review-
Comment 4•8 years ago
|
||
(In reply to :Ehsan Akhgari from comment #3)
> > +SpaceAfterTemplateKeyword: false
>
> Don't you want true for this one?
There doesn't seem to be any in-codebase consensus. For example, looking at RefPtr.h, there are both space-after-template and not-space-after-template within a couple lines of one another. (http://searchfox.org/mozilla-central/rev/b9c9d257366379ac984622dbef38432a7fecd2e9/mfbt/RefPtr.h#33-46) In the coding style document, the `template` keyword is used with no space after it (https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Methods).
I don't think it's super clear cut but I could totally see why Sylvestre would choose no space after keyword.
Assignee | ||
Comment 5•8 years ago
|
||
I tried to rerun on the whole source code with SpaceAfterTemplateKeyword: true
and the change is not big:
320 files changed, 1125 insertions(+), 1007 deletions(-)
What would you prefer ehsan?
Flags: needinfo?(ehsan)
Comment hidden (mozreview-request) |
![]() |
||
Comment 7•8 years ago
|
||
I have always thought no-space-after-template was the preferred coding style, even without reading the coding style document.
Comment 8•8 years ago
|
||
I honestly don't care that much... Since it seems that Nathan has a slight preference, I can be swayed that way. :-)
Flags: needinfo?(ehsan)
Updated•8 years ago
|
Attachment #8817111 -
Attachment is obsolete: true
Attachment #8817111 -
Flags: review?(ehsan)
Comment 9•8 years ago
|
||
r+ on the original patch. Not sure how to make MozReview do the right thing here.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 11•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/38470e52b9d3beb7f648d8726d0c731684ddb75c
Bug 1322321 - Update the clang format file to match more our coding style r=ehsan
Assignee | ||
Comment 12•8 years ago
|
||
Also committed upstream: https://reviews.llvm.org/rL289660
Should be in clang 4.0
Comment 13•8 years ago
|
||
Comment on attachment 8817111 [details]
Bug 1322321 - Update the clang format file to match more our coding style
More MozReview confusion.
Attachment #8817111 -
Flags: review?(ehsan)
Comment 14•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Assignee | ||
Updated•7 years ago
|
Attachment #8817111 -
Attachment is obsolete: true
Updated•7 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
•