Update the clang format file to match more our coding style

RESOLVED FIXED in Firefox 53

Status

defect
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: sylvestre, Assigned: sylvestre)

Tracking

(Blocks 2 bugs)

Trunk
mozilla53
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(1 obsolete attachment)

Assignee

Description

3 years ago
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

3 years ago
Assignee: nobody → sledru
Assignee

Comment 2

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

Comment 3

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

3 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)
I have always thought no-space-after-template was the preferred coding style, even without reading the coding style document.

Comment 8

3 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

3 years ago
Attachment #8817111 - Attachment is obsolete: true
Attachment #8817111 - Flags: review?(ehsan)

Comment 9

3 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

3 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

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

Comment 13

3 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

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/38470e52b9d3
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Assignee

Updated

2 years ago
Attachment #8817111 - Attachment is obsolete: true

Updated

a year ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.