Closed
Bug 961541
Opened 10 years ago
Closed 7 years ago
Make clang-format more compliant with Mozilla style
Categories
(Developer Infrastructure :: Source Code Analysis, defect)
Developer Infrastructure
Source Code Analysis
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: ajones, Assigned: poiru)
References
Details
Attachments
(1 file, 1 obsolete file)
317 bytes,
patch
|
Details | Diff | Splinter Review |
Get clang-format changes accepted upstream. Once these are accepted, use a locally installed clang-format of the appropriate version.
Reporter | ||
Comment 1•10 years ago
|
||
Reporter | ||
Comment 2•10 years ago
|
||
Reporter | ||
Comment 3•10 years ago
|
||
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.
Comment 4•10 years ago
|
||
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.
Comment 5•10 years ago
|
||
(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)
Comment 6•10 years ago
|
||
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)
Comment 7•10 years ago
|
||
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)
Comment 8•10 years ago
|
||
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.
Comment 9•10 years ago
|
||
(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.
Reporter | ||
Comment 10•10 years ago
|
||
(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.
Assignee | ||
Updated•9 years ago
|
Attachment #8362289 -
Attachment is obsolete: true
Assignee | ||
Comment 11•9 years ago
|
||
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
Assignee | ||
Comment 12•9 years ago
|
||
http://reviews.llvm.org/D10774 (Add option to break after definition return type for top-level functions only)
Assignee | ||
Comment 14•9 years ago
|
||
Clang-Format doesn't reflow comments properly so I went ahead and made it ignore all comments. See commit above.
Keywords: leave-open
Reporter | ||
Comment 16•9 years ago
|
||
(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/
Assignee | ||
Comment 17•9 years ago
|
||
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.
Comment 18•9 years ago
|
||
poiru++
Assignee | ||
Updated•9 years ago
|
Blocks: clang-format
Assignee | ||
Updated•8 years ago
|
Component: Build Config → Rewriting and Analysis
Comment 19•7 years ago
|
||
Is there anything left to do here? Can we enumerate the style differences remaining?
Flags: needinfo?(ajones)
Reporter | ||
Comment 20•7 years ago
|
||
(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)
Comment 21•7 years ago
|
||
Sylvestre should we just close this? Seems like the work has moved to other bugs.
Flags: needinfo?(sledru)
Comment 22•7 years ago
|
||
yeah, let's close it
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: needinfo?(sledru)
Resolution: --- → FIXED
Comment 23•6 years ago
|
||
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
Updated•6 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
•