Closed Bug 1322321 Opened 4 years ago Closed 4 years ago

Update the clang format file to match more our coding style

Categories

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

defect
Not set
normal

Tracking

(firefox53 fixed)

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: Sylvestre, Assigned: Sylvestre)

References

(Blocks 2 open bugs)

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.
Assignee: nobody → sledru
Change also proposed upstream:
https://reviews.llvm.org/D27557
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-
(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.
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)
I have always thought no-space-after-template was the preferred coding style, even without reading the coding style document.
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)
Attachment #8817111 - Attachment is obsolete: true
Attachment #8817111 - Flags: review?(ehsan)
r+ on the original patch.  Not sure how to make MozReview do the right thing here.
https://hg.mozilla.org/integration/mozilla-inbound/rev/38470e52b9d3beb7f648d8726d0c731684ddb75c
Bug 1322321 - Update the clang format file to match more our coding style r=ehsan
Also committed upstream: https://reviews.llvm.org/rL289660
Should be in clang 4.0
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)
https://hg.mozilla.org/mozilla-central/rev/38470e52b9d3
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Attachment #8817111 - Attachment is obsolete: true
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.