1322321 - Update the clang format file to match more our coding style

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[Sylvestre Ledru [:Sylvestre]]

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 1

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1322321 - Update the clang format file to match more our coding style

Review commit: https://reviewboard.mozilla.org/r/97540/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/97540/

Comment 2

[Sylvestre Ledru [:Sylvestre]]

Change also proposed upstream:
https://reviews.llvm.org/D27557

Comment 3

[(no longer active)]

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?

Comment 4

[Nika Layzell [:nika] (ni? for response)]

(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.

Comment 5

[Sylvestre Ledru [:Sylvestre]]

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?

Comment 6

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8817111 [details]
Bug 1322321 - Update the clang format file to match more our coding style

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97540/diff/1-2/

Comment 7

[Nathan Froyd [:froydnj]]

I have always thought no-space-after-template was the preferred coding style, even without reading the coding style document.

Comment 8

[(no longer active)]

I honestly don't care that much...  Since it seems that Nathan has a slight preference, I can be swayed that way.  :-)

Comment 9

[(no longer active)]

r+ on the original patch.  Not sure how to make MozReview do the right thing here.

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8817111 [details]
Bug 1322321 - Update the clang format file to match more our coding style

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97540/diff/2-3/

Comment 11

[Sylvestre Ledru [:Sylvestre]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/38470e52b9d3beb7f648d8726d0c731684ddb75c
Bug 1322321 - Update the clang format file to match more our coding style r=ehsan

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Also committed upstream: https://reviews.llvm.org/rL289660
Should be in clang 4.0

Comment 13

[(no longer active)]

Comment on attachment 8817111 [details]
Bug 1322321 - Update the clang format file to match more our coding style

More MozReview confusion.

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/38470e52b9d3