Closed Bug 961541 Opened 6 years ago Closed 2 years ago

Make clang-format more compliant with Mozilla style

Categories

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

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ajones, Assigned: poiru)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Get clang-format changes accepted upstream. Once these are accepted, use a locally installed clang-format of the appropriate version.
I've had the following feedback in http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20140120/097563.html which shows our style guide perhaps needs some clarification.
Do you mean the comment about ContinuationIndentWidth?  Yeah, I'm not sure if our style guidelines cover that.  Benjamin, do you know what we should do there?

It seems to me like the comment about IndentCaseLabels is correct, you should leave that set to true.
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #4)
> Do you mean the comment about ContinuationIndentWidth?  Yeah, I'm not sure
> if our style guidelines cover that.  Benjamin, do you know what we should do
> there?

Forgot to ni? Benjamin.
Flags: needinfo?(benjamin)
ContinuationIndentWidth means the indent that happens for a line that is incomplete? What cases does this cover?

  int a = SomethingReallyLong + SomethingElse +
    SomethingElse; // 2 sounds reasonable

  v =  a(int a, int b, int c, int d,
         int e, int f); // current common practice indents this to the opening paren, except when that would
                        // lead to very long lines
Flags: needinfo?(benjamin) → needinfo?(ehsan)
Given this test case: <https://github.com/llvm-mirror/clang/blob/76f9c8295f513bd37bbcf6a90adef4478ccc22b6/unittests/Format/FormatTest.cpp#L8009> I think it applies to both.  IOW, it applies to all line continuations.
Flags: needinfo?(ehsan)
Well hrm. Without more smarts, I think "2" is the best we can specify here. I'm a little worried about the readability of multi-line functions arguments, but lets try it.
(In reply to comment #8)
> Well hrm. Without more smarts, I think "2" is the best we can specify here. I'm
> a little worried about the readability of multi-line functions arguments, but
> lets try it.

Yeah, I agree.  Anthony, can you please check to see if the clang folks plan to add another continuation option to handle "callable things" such as functions/macros/etc. differently?  If not, we can stick to 2 for now.
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #6)
> ContinuationIndentWidth means the indent that happens for a line that is
> incomplete? What cases does this cover?
> 
>   int a = SomethingReallyLong + SomethingElse +
>     SomethingElse; // 2 sounds reasonable

This is where the 2 comes in. It seems that 4 is a common value on some projects.

>   v =  a(int a, int b, int c, int d,
>          int e, int f); // current common practice indents this to the
>                         // opening paren, except when that would
>                         // lead to very long lines

clang-format does a reasonable job of this case, however it is mechanical so it does make different decisions than a human would.
Attachment #8362289 - Attachment is obsolete: true
Encouraged by the recent dev-plaform discussion[0], I have started working on patches to make clang-format more compliant with our style guide. I'll post links to the patches as I upstream them. First one:

http://reviews.llvm.org/D10771 (Adjust Mozilla style defaults)

[0]: https://lists.mozilla.org/pipermail/dev-platform/2015-June/010515.html
Assignee: nobody → birunthan
Status: NEW → ASSIGNED
Summary: Upstream clang-format changes and prefer installed version → Make clang-format more compliant with Mozilla style
http://reviews.llvm.org/D10774 (Add option to break after definition return type for top-level functions only)
Clang-Format doesn't reflow comments properly so I went ahead and made it ignore all comments. See commit above.
Keywords: leave-open
(In reply to Birunthan Mohanathas [:poiru] from comment #11)
> Encouraged by the recent dev-plaform discussion[0], I have started working
> on patches to make clang-format more compliant with our style guide. I'll
> post links to the patches as I upstream them.

\o/
Recent commits:
* Add Mozilla brace breaking style (http://reviews.llvm.org/rL241986). This makes Clang-Format break before enum/union/struct braces.
* Add Block{Begin,End}Macros option (http://reviews.llvm.org/rL241363). This allows us to teach Clang-Format about e.g. NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN and friends.
poiru++
Blocks: clang-format
Component: Build Config → Rewriting and Analysis
Is there anything left to do here? Can we enumerate the style differences remaining?
Flags: needinfo?(ajones)
(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #19)
> Is there anything left to do here? Can we enumerate the style differences
> remaining?

I haven't looked at this in 4 years so I'm the wrong person to ask.
Flags: needinfo?(ajones)
Sylvestre should we just close this? Seems like the work has moved to other bugs.
Flags: needinfo?(sledru)
yeah, let's close it
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Flags: needinfo?(sledru)
Resolution: --- → FIXED
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.