RFC: Clang-format for Blender #53211
Labels
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
13 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#53211
Loading…
Reference in New Issue
No description provided.
Delete Branch "%!s(<nil>)"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
UPDATED JAN 7/2019 - Ultimately this proposal will do two things
Overview
Currently the code in Blender has a style guide, but there are only limited tools to enforce this style. In the past, there have been various tools for reformatting C and C++ code, but for the most part they did not work well. This was usually because they did not implement a full C/C++ parser, and instead tried to parse a subset of the language in order to provide the formatting. Unfortunately, this only works for simple code; the moment sophisticated code is used, these simplistic formatting solutions don't work well.
In the intervening years, a new entrant has stepped onto the stage: clang-format. clang-format is different than the previous versions for a key reason: it is built around a full-scale industrial strength C/C++ parsing engine, clang. And unlike other attempts, clang-format is used and extended by some of the largest companies for their large scale codebases: Apple, Google, Mozilla, and more.
Additionally, beyond just leveraging the full C/C++ parser of Clang, clang-format aims to support a limited number of formatting styles, but support those styles extremely well; handling as many edge cases correctly as possible, to avoid spending engineering time fixing autoformatting mistakes.
For all these reasons, this proposal is to figure out a plan to bring clang-format to Blender.
Why have consistent formatting?
The key reason is to have consistent code that is readable by all Blender developers, and to help save time by removing time spent debating style questions.
Why switch to automatic formatting?
The simple reason is that it's a waste of precious developer time to have them obsess over formatting details. In the past, automatic tools did not work well enough, but that is no longer true. Let's both save time and increase code quality by leveraging the immense engineering effort of the clang format team to increase Blender's code consistency.
Proposed rollout plan
.clang-format
file to Blender's repository (probably root Blender repo). Note that clang-format doesn't work well in an external repo; it does not support specifying a style file, and instead scans directories recursively upward from a file to find the closest parent directory with a .clang-format file.Eventually, after the version of LLVM is updated to the latest, switch to using the clang-format that's included in the libs (probably going to clang-format 6.0 if possible).
I want to try it!
See the below sample config and instructions.
UPDATE JAN 7/2019: See the new
temp-clang-format
branch created by Campbell; it has the current source-of-truth version of the.clang-format
.Guidelines for using clang-format
End-goal: running clang-format on a file will result in no changes to that file. This will not always be possible due to some limitations in clang-format, but this is the desired result.
clang-format -i <filename>
on the file you are working on.git diff
or git integration with your editor, examine the changes introduced by clang-format to make sure they are acceptable./* clang-format off */
and/* clang-format on */
as needed, or use//
to prevent line merges (see below for details)Do / Do not
Issues with clang-format
While clang-format is comprehensive, not everything can be set to match Blender's style, and additionally there are some minor bugs that can crop up.
{
. There is no way to optionally break before a{
if theif
orfor
body is more than one line. Adding this would require making upstream changes to clang-format.#ifdef
line. Unclear why.,
assignments seems to be a bit off.Long term we'd like to fix these upstream, but upstream fixes will take a long time to propagate through to a stable release. So for now, unfortunately these issues will have to be manually addressed.
FAQ
#
issue with the preprocessor and (b) the newline before{
forif
statement conditions that span multiple lines. (a) is already supported in upstream clang-format, but it is not available everywhere yet without compiling LLVM (a big job). (b) could is not implemented, but we could probably convince upstream to take a patch for that. UPDATE JAN 7/2019: This is no longer an issue since Clang >=6 is widely available now.{}
everywhere even for one-line statements?sudo apt-get install clang-format
. On Windows, it is available as part of the [Windows builds](https://llvm.org/builds/ LLVM nightly).Editor integration
Config
The configuration options are documented at the LLVM site.
Proposed config for Blender that gets most of the clang-format settings pretty close is below. To use it, create a file
.clang-format
in the root Blender repo (next tomake.bat
). Then runclang-format -i <file>
.UPDATE JAN 7/2019: See Campbell's
temp-clang-format
branch for the latest.clang-format
config.Changed status to: 'Open'
Added subscriber: @Keir
Added subscribers: @mont29, @ideasman42, @brecht, @dfelinto, @Sergey, @fclem
@Keir, some questions:
I think it's really great to have something more sophisticated than a current format script. Especially, as long as we do not encourage to apply this globally, but rather via module owners.
I can't find
clang-format
in my Xcode 9 install?For macOS and Windows we can install it along with precompiled libraries. In fact it's already there on Windows, but that's from LLVM 3.5. We will need to upgrade to a newer LLVM/Clang version, which is needed for newer OSL anyway.
Try
brew install llvm
; that should getclang-format
. Agreed the best path forward would be to bundle clang format as part of the libraries on all platforms; that way it's easy for everyone to run, and the version of clang format will be fixed.Think its worth considering this, although I have concerns.
source/
(maybe move towards applying globally, but at least not to begin with).this
becomes
this
becomes
clang-format
5.0, First file I tried ended up indenting the last 70 lines an extra single tab indent (bmo_inset.c
).Examples of code which might loose alignment.
eg:
Tests from
bmo_inset.c
before:
after...
before
after
before
after
interface_handlers.c
before
after
before:
after
Response to Campbell's questions:
.clang-format
file in the directory. It's not all-or-nothing. My proposal to start is just to commit the.clang-format
file and allow module owners to use it or not at their discretion.-style=google
and re-format everything, but that's why I'm not in charge!/* clang-format on/off */
to disable it for blocks, and you can insert a//
at the end of a line to prevent clang-format from merging two lines (sometimes useful for stacked expressions where clang-format would get it correct except for a line merge).Thanks for the comprehensive reply, checked some more.
Do we need this?
Giving own opinion on this question.
I run a style checker once every month or so and find a handful of issues from time to time, it normally takes under 10min to resolve.
There are many areas of Blender's code that rarely change (where most commits are for maintenance), so the cost of having slightly less readable code, compared to the time spent to to manually maintain style - makes auto-formatting less of an obvious win.
Said differently, if we had a lot of code-churn, auto-formatting would be more appealing.
Main gain is for new code & patch review, where IMHO it's a "nice to have".
Blockers
Before going into details, my overall impression is we should not be having to riddle our code with
/* clang-format off */
to keep it generally readable.Here are issues I consider blockers (or borderline cases).
Brace Placement w/ Multi-Line Checks
While this may seem unimportant, most aspects of code style are personal preference, to me this is a choice that makes flow control more readable, especially when these cases are typically more involved to begin with.
The ability to see flow-control at a glance is important for readability, scanning code quickly - which IMHO makes this more significant than a simple style choice.
Side-note, I read google use 2 spaces for indentation, which avoids accidental alignment seen in the examples below.
If these were some isolated cases, I'd accept a handful of exceptions, but AFAICS they're not that rare.
eg:
before:
after:
before:
after:
before
after
Function Arg Wrapping
There are enough parts of Blender's API that have many function args (maybe code should be refactored to avoid this.)
There are enough places this causes heavily wrapped function calls, which we would reject if someone submitted in a patch.
eg:
Note that the solution here might just be to have multiple arguments on the same line? - So maybe not a blocker.
IMHO one argument per line is fine in many/most situations, there are just enough cases that expand to over ~8 lines which IMHO is getting a bit ridiculous.
Preprocessor
For me this is borderline blocker (think its a shame to loose this ability and don't see why clang-format couldn't just leave pre-processor lines alone)
OTOH there are not that many complex ifdef blocks, so we could explicitly disable formatting on the areas that really need indentation to be readable.
before:
after
Indentation of Arrays
before:
after: (for some reason it's using 8 spaces indentation... maybe simple to solve)
Non-Blockers
Some practical concerns which we can work-around:
Bugs in Clang-Format
bgl.c
uses > 10gig of memory when formatting. (I've noticed this with clang's parser in some of Blender's files that use complicated macros).NOD_static_types.h
does some real strange indenting P551 (we'd just have to disable for this file)Noisy Changes (after upgrading clang-format)
If we upgrade clang-format, any changes in its behavior may make for noisy diff's.
Suppose there is no good solution here (just view this as a minor down-side).
We could always postpone re-formatting.
Comment Wrapping
There is excessive re-wrapping of comments, even when they don't exceed the line length.
Mostly it's harmless, but sometimes it meddles with text that would be better left as-is.
Seems we can avoid this by setting
ReflowComments: 'false'
Distributing Clang-Format
Everyone having the same version of
clang-format
, while possible, doesn't come for free.Complex tools like this add maintenance & support overhead.
Conclusion
Overall I'm not convinced of this proposal being a net gain (at this moment),
while I'd be happy to accept some compromise, there are some issues which for me are blocking and should really be resolved before this could be applied even on a case-by-case basis.
The only serious blocker from my POV is brace placement, other issues seem like they could be resolved via minor changes or overlooked in the case of pre-processor indentation.
Added subscriber: @LazyDodo
before we get all excited and go we can upgrade clang-format with never versions and solve all our problems, the only reason we currently have llvm/clang in our libs, is because it's a requirement for OSL. Which i assume will stay the leading dependency for clang, so unless OSL supports it (and they have been lagging with supporting new versions in the past by quite a bit) there will be no upgrades to clang-format either. something to probably keep in mind
@LazyDodo not sure what you mean about "solving all our problems" ? - I've been testing the latest release of clang-format v5.0.
there seems to be a general vibe of, 'there is some issues with the current version, but if we submit some patches a future version could handle all our cases that are currently broken' i'm just pointing out that some of our other deps might be holding back updating to the latest and greatest.
OSL was stuck on an old LLVM version for a long time, because LLVM completely rewrote the JIT compiler to share code with the regular compiler. There were performance issues with that, and they have been solved now. It should be ok to upgrade to LLVM 5.0, and I don't expect major issues upgrading to newer versions in the future.
Added subscriber: @Stefan_Werner
Just talked with mont, dalai & sergey, we're thinking of trying out something closer to googles code style since it solves the accidental allignment of if-statements.
Differences are:
P560
also made other modifications:
A note on argument wrapping, passing 10+ arguments to a function isn't great.
In the case of interface code, there is x/y/w/h which could be written as a compound struct,
Of course using this only to avoid splitting arguments over too many lines isn't a good reason to do so.
Remaining issues:
before:
after:
While not happy about it, I do can live with 2-spaces indentation and 80 chars width. But I still think this is less readable, 2-spaces make it much harder to tell at which level, i.e. which block, you are, and 80 chars is very small, especially when using longer, more verbose/descriptive names. What you gain in width, you lose in high, not a winner trade imho. Especially since it means you see much less real code at once on your screen, and have to scroll even more.
Granted, we’d have to adapt to new style, readability also is a matter of taking it into account when writing code. But… Here are a few examples which I really find much, much worse after clang-format (using @cambellbarton's one, and clang-format 3.8, which is default one on Debian testing):
giving…
giving…
giving…
And even with all those lines with only
{
on it removed, file still goes from 665 lines to 724…So to summarize, am not convinced, and do not even understand how a style-fixing tool can miss a point like 'put opening brace on next line for multi-lines statements'… For me this is very 101 of readability, much, much more than putting one arg per line in function calls or declarations! Unless you simply forbid any complex if/for/etc. statement, and enforces to do any complex handling in separate steps. But this kind of goes beyond code style, and certainly cannot be handled by automatic tool anymore.
@mont29 - agree with your concerns.
Brace placement is important for readability - we could hold off using clang-format until this is handled, check if it's something they would support.
Update, tested clang6 and compared output with clang5, it improved on indenting some comment blocks. Otherwise issues we ran into aren't resolved.
(some of them should be reported to clang, also it's possible there are new options in clang6).
Reported bug to clang-format regarding C99 struct alignment: https://bugs.llvm.org/show_bug.cgi?id=37134
Looks like we can work aorund the C99 struct alignment issue by setting
BreakBeforeBinaryOperators: All
@ideasman42 if we have a trailing comma in all the initialized struct members the result is far better (in this case I added a comma after the
}
of.exit_code_on_error
).I talked to @ideasman42 and he proposed we had a post-processing script to handle the cases where clang-format falls short. This would allow us to set a code style for Blender that will still be valid even after clang-format supports some of our requirements. Over time our goal would then be to reduce the requirement of the post-processing script as clang-format gets fixed.
My concern is only with this conflicting with the clang-format support for IDEs. I wonder what @Keir thinks of it.
PS.: Interesting read up on MongoDB clang-format adoption - https://engineering.mongodb.com/post/succeeding-with-clangformat-part-1-pitfalls-and-planning/
And then have script which handles failures of post-processing script...
The whole idea of clang-format is to make it simple to bring source to a consistent state, without any extra work from our side, utilizing support of IDEs as much as possible.
Are we really going to write own fully-featured C11 parser, with support of preprocessor and such? Using Python 3, requiring Windows users to have it installed in PATH? Or use clang lexer python bindings, which i do not see existing for python3.
I do not find the idea of a separate script correct at all.
msvc has build in clang-format support nowdays, but doesn't offer any post processing options, so i really would prefer not to go down the scripting route.
Assuming we do no post-processing then. A direct comment on @ideasman42 's blockers:
Brace Placement w/ Multi-Line Checks
If we switch to 2-space indentation this problem disappear.
Function Arg Wrapping
I don't think is an issue at all. The problem comes from our (ab)use of extremely short variables. In fact this even stimulates the developer to use more clear variable names.
Preprocessor
In this case I would say just use
/* clang-format off */
Array-indentation
Not sure what is the problem here. The 8 indentation?
Given 2.8 has finally made it to master, is there any chance we can get this moving again?
This enforces all operators to be on a newline, which I'm not a fan of (often mis-aligns statements that would otherwise be aligned, goes against current convention) - giving us less readable code to workaround a clang bug.
See docs: http://releases.llvm.org/7.0.0/tools/clang/docs/ClangFormatStyleOptions.html
Agreed (although still find braces on newlines more readable in this case).
Ok, mainly the UI code gives awkward wrapping, we could use structs more in this case, eg:
rcti
instead ofx,y,w,h
.This would mean disabling clang-format for most pre-processor heavy blocks.
Yes, it's just strange that array definitions use different indentation to other blocks. Not sure if there is a way to indent arrays the same as regular code blocks? Without having
ContinuationIndentWidth
set toIndentWidth
.Some inlined answers
Would absolutely love to!
I would consider this as a point against of heavy preprocessor blocks. There are only handful of those which are useful, the rest are just causing extra overhead for debugging.
It seems the arguments against this proposal are based on the behaviour of clang-format on a code which is already not very good. To me the way to deal with this would be:
On a bigger scale this is how i see this proposal:
Pros:
Cons:
Personally, i value the time for actual development gained from automated technical task of ensuring code style more than keeping old ugly code in its bubble.
Just some examples of us(me) spending a lot of time on this topic:
@ideasman42, are you still opposed to this approach? From reading the comments, it appears you are the primary detractor.
While on one hand, I'm happy to see that this issue is not closed with "no way!" yet, I'm also sad that there still isn't a resolution after more than a year. There is a strong dollars-and-cents case for automatic formatting, which is why so many companies have adopted it. For a volunteer project such as Blender, where core developer time is incredibly valuable, and onboarding new developers is critical to the lifeblood of the project, I hoped it would be clear that adopting something like this proposal is worthwhile. While there are edge cases that may require some compromise or a sprinkling of
/*clang-format: off */
, it's important to not over-weight the small up-front cost of this versus the high and ongoing tax paid by the core developers to personally enforce style.Do not agree. Switching to two-spaces indentation is imho very bad for readability, makes it kind of annoying to detect at which level of indent you are after a few lines, this is not visually obvious anymore… And this is a very poor band-aid for the actual issue. TBH, I’d rather enforce always putting opening brace on newline, would make files a tad lengthier, but still much better than current clang code-style "fix" on that matter. And then no need to switch to horrible two-spaces indent!
@Keir @Sergey I totally agree automatic formatting is a great tool to have - as long as it does produce good readable code. ;)
Also another bad thing about two-spaces indent: it encourages people putting even more levels of sub-blocks into a single function (since they do not 'cost' much space anymore), which is typically bad practice. The bigger the indent, the more obvious it becomes when you go beyond reasonable level (which is something like 4 or 5 in most cases I think…).
Probably we agree. I was only agreeing that it solves the spesific problem mentioned to begin with, not advocating for 2 space indentation, although I don't have a strong opinion - especially if we're going for 80 char width limit.
+1 for braces always on newlines, as an alternative.
+1
It's not clear to me whats being proposed now, it seems we'll have to make some compromises. Ok, which ones?
One sticking point is a fairly obvious problem with struct alignment which I reported and have no reply for 8 months.
One proposed solution is to workaround this by setting
BreakBeforeBinaryOperators: All
which has much wider style implications which IMHO make code less readable (code which has nothing to do with struct declarations).We can just accept this too, but it's not great that we have to accept too many trade-offs, especially if - for all we know, they get fixed in the next release.
Agree with your sentiment here.
Tried tweaking options and here are two which I think are acceptable.
If we keep 4 space indentation I don't think 80 char width limit is reasonable.
Suggest either of these:
Notes:
ForEachMacros
in.clang-format
above need updating./* multi-line-code-blocks */
will need a one time manual fix since they aren't getting properly re-indented (unless clang-format has a way to handle it).+1 for 4 char indent and conditional braces on new line.
We could relax the rule of always requiring conditional braces, and only require them if them if the condition does not fit on a single line? Would gain back a bit of space, and with auto formatting you can't accidentally indent wrong.
Personally I like a wider with, though maybe 100 instead of 120 is a better compromise for side by side code viewing?
Also think this could be added:
ForEachMacros
should have'foreach'
as used in Cycles.This would be ideal, the issue AFAICS is there is no way to tell clang-format to do this.
100 is fine, wouldn't want lower for 4-space indent though (Rust's convention is <=99, found it OK).
+1 for both. updated P884 config, although many more foreach macros are needed.
Also committed trailing commas to workaround strange struct indentation
e305560f13
Also +1 for the 4 char version.
ForEachMacros
is also missing some from BLI, at leastLISTBASE_FOREACH
.The ones with
BEGIN
/END
are not concerned here I guess? LikeFOREACH_PCHAN_
,FOREACH_COLLECTION_
,FOREACH_SCENE_
, etc.Regarding width, am not fully against 80 chars, but I think even 100 chars should be more than enough short for side-by-side display on any modern screen? At least that’s what I can do, without even occupying the whole screen width, on my tiny laptop FullHD screen. ;) So that’s a compromise which sounds reasonable to me.
Updated P884 - 100 width (wrap at 99), added
LISTBASE_FOREACH
, although there are probably 20+ looping macros to add here.Edit: Since
AfterControlStatement: 'true'
is set, Values ofForEachMacros
have no effect.I updated P884 and P885 to turn on
IndentPPDirectives
, which is now commonly available in deployed Clang versions (available as of Clang 6). This improves preprocessor formatting.I am not in a favour of using any of *OnASingleLine family of settings enabled. They are making code more difficult for debugging (and to my knowledge even goes against existing rule of not putting things ono a single line).
Right,
clang-format
can't add or remove braces. I just meant for the guidelines that we follow manually.It seems to have an effect when there are no braces.
I'm fine with
None
if you prefer typical getter/setters in class bodies to be on multiple lines, I don't feel strongly about that. Mainly I wanted to change it from the defaultAll
.Looks like clang-tidy may be able to do this , not ideal, but we'd have to run it once during migration to the new rules so it may not be that bad...
Not keen on moving away from using braces (where braces use too much vertical space there is some incentive not to use them).
This brings back the annoyance of lines from the statement and it's body running into each other, eg:
Becomes:
I suppose maintainers of each area can choose when to do this as they do now.
We would need to use
do {...} while (0)
in macros instead of{...} ((void)0)
if we want to use them w/o braces in conditionals.Don't feel strongly either fine w/ None, or Inline (edited P884).
edit, IndentPPDirectives now works
In general P884 LGTM (accepting there are some less than ideal trade-offs we can't avoid).
I didn't check the output in detail for C++ code, so I'm assuming others are OK with this configuration.
Any remaining blocking issues?
Otherwise, next steps.
source/
andintern/
) - althoughintern/cycles
might be handled in a separate pass.@brecht,
Using explicit getters/setters for a trivial assignment (without any sanity checks i.e.) is often turns out to be more of a burden than helpful in comparison with direct access. But this is another discussion anyway.
@ideasman42, is just
source/
with a selective directories inintern/
, like,guardedalloc
.intern/
only means that it's an external library maintained by Blender developer. There is no need to enforce Blender's code style globally there.Committed branch
temp-clang-format
w/ initial migration scripts, see: https://developer.blender.org/diffusion/B/browse/temp-clang-format/These files will move, to try it out, files added are:
.clang-format
(from P884)clang-format-migration.sh
converts tabs to spaces before re-formatting.clang-format-edited.sh
runs on modified files only.Global conversion from tabs to spaces could backfire (strings that contain non-literal tabs for eg) We could replace all tabs in strings with
\t
literal before applying clang-format.Current migration breaks makesdna by splitting:
into
Makesdna could support this but we wont want this formatting, suggest to put comments on line above struct member.
Let's not forget clear instructions for new and old developers: updated code-style, IDE, and build wiki documents.
This is great! Nice progress.
@ideasman42 - Why the need for the custom replacement of tabs with spaces in Python? clang-format handles that natively.
@dfelinto - Agreed there will be a long tail to this migration. First step is just to get to a
.clang-format
that we mostly agree on, and we're nearly there!Regarding the
makesdna
issue: I have a suggestion: switch the SDNA coding standard to put comments on the line before. This fixes the formatting problem and also encourages longer and more detailed comments. A side note, my personal feeling is that not all code is created equal; some code is more core to a product/project then others, and so deserves a very high level of variable naming and commenting rigour. In my opinion, Blender's SDNA is in this category and deserves to have the most obnoxiously high quality code standards, including naming and in-code documentation (comments).Existing:
Suggested:
@Keir moved comments above struct members (preferred this anyway, multi-line comments became annoying to wrap and properly indent).
5a43406e1b
The clang-format result now builds.
The reason to use expand tabs before running clang-format is clang-format isn't touching comment blocks after the first line (unless I miss some option to tell it to expand tabs but otherwise not re-wrap them).
Added subscriber: @dr.sybren
You no longer need to grab llvm to get clang-format on windows, visual studio 2017 ships with clang-format support out of the box, the ide will detect the file and use it for it's auto formatting support. However given we currently have both a .clangformat (which says use spaces) and a .editorconfig (which says use tabs, also missing the .cpp file type btw) it's acting kinda bizarre, it inserts tabs, but when you manually run code formatting it changes them to spaces.
@LazyDodo Good catch on the
.editconfig
discrepancy. I've extended #60279 to include updating other IDE configs that exist in the Blender source.Added subscriber: @JulianEisel
Why do we have
ConstructorInitializerIndentWidth: 0
? Personally i find this hard to follow (and i'm one the few who spends a lot of time in C++ parts of Blender..)Can we have at least single, or double indentation for it?
@Sergey, I'm fine if
ConstructorInitializerIndentWidth
is changed. Probably it was set like that to follow existing code style.Running
clang-format
on.glsl
and.osl
code seems to work. I've enabled it intemp-clang-format
..editorconfig
intern/ghost
,intern/clog
Some other points:
tabs -> spaces
everywheremakesrna.c
, clang-format wont handle this so needs to be done manually.Added subscriber: @JacquesLucke
Added D4185 some TODO's noted:
make format
can be used to try out formatting.Clang detects header guards which mostly works, but there is a case it fails.
If this is anywhere in the header, indent guard detection fails.
Applies to:
./intern/clog/CLG_log.h
./intern/guardedalloc/intern/mallocn_intern.h
./source/blender/blenlib/BLI_compiler_compat.h
edit: reported https://bugs.llvm.org/show_bug.cgi?id=40288
edit: worked around
302970b7a5
I've added most
intern/
modules for formatting now intemp-clang-format
, including Cycles. The only excluded modules still are libmv, numaapi (using Google style) and elbeem, smoke, itasc (originated outside of Blender).This is still up for discussion, just my guess for what the relevant maintainers would want.
@brecht, opensubdiv's C-API is also following google's style.
Ok, I've excluded opensubdiv as well now.
One nifty thing:
git-clang-format
from [1].From a quick tests it does reformat on modified hunks (the files are to be staged first). Seems easy to distribute, and probably something to encourage people to setup as a pre-commit hook?
Added
tests/gtest
, removedBLI_ressource_strings.h
since it's a datafile.Also removed
BLI_compiler_typecheck.h
&BLI_utildefines_variadic.h
- since results aren't acceptable and not something we're likely to be able to tweak.+1, we'll need to filter out files that use too much memory though
./intern/cycles/render/sobol.h
hangs on my system, even when using// clang-format off
.Another thing i'd love to propose is to enable
FixNamespaceComments
.Update:
FixNamespaceComments
~ seems generally helpful.clang-format off
instead of excluding files (except forsobol.h
).I don't see any blockers for migrating to clang-format, besides more general docs, pre-commit hooks etc. which we might want to have before migrating.
Having said this, we might want to disable formatting in more files based on closer inspection of the output.
./source/blender/nodes/NOD_static_types.h
wraps some lines and not others in a way that doesn't read well.snippet:
There are also many comments at the ends of lines that cause strange formatting.
Eg:
Becomes:
We could let clang-format wrap comments, making sure all ascii diagrams don't exceed the line length before formatting - or disable formatting when we can't edit to fit the width, see:
./source/blender/compositor/COM_compositor.h
.Edit, both issues have been handled, however similar issues remain elsewhere.
I've been going over the output of
make format
, disabled formatting for a handful of areas where it's not helpful (mostly large definitions which read much better when hand aligned). See: D4185.It would be good for other maintainers to try running
make format
in thetemp-clang-format
branch and check the results are acceptable so we can move forward with this task.Added subscriber: @Jeroen-Bakker
I have gone over the compositor and workbench engine. In the compositor I only found minor stuff what is is not needed to be mentioned. In the workbench engine I did found something that we might want to fix later on.
The workbench engine uses a lot of one-line defines to disable/enable features in the new format these are no one liners anymore.
I agree that the formatted is more clear. and a developer knows that more lines needs to be altered.
I don't see that we need to change this, as it makes the code more readable and we are talking about seconds per incident. And normally being done by someone who is familiar with the code.
Pro: Code is better readable!
Also a note .cl files are not formatted, but should be doable as they are based on C.
All in all, am okay with it… still not enchanted (by far ;) ), but it's okay. We can always work around stupidities like that:
for (ancestor_id = reference_id; ancestor_id != NULL && ancestor_id->override_static != NULL &&
ancestor_id->override_static->reference != NULL;
ancestor_id = ancestor_id->override_static->reference)
;
…by avoiding coder's own stupidity and rewriting things a bit to avoid that kind of lengthy statements eg. :D
One thing I noted though, it does not seem to wrap comments to follow the 99 char rule?
@mont29 also sub-enchanted here. There are many blocks of code that are less readable after applying clang-format, its more a trade-off - we accept some less readable code to get the advantage of not having to fuss w/ formatting it. Disabling in extreme cases.
Formatting comments is disabled because we have enough comments that mix in ascii diagrams, dot-points, code snippet... etc, that we better manually wrap lines there.
We could handle it differently - find all comment blocks that use diagrams and disable clang format for those, but going over all comments is a fairly big task.
@ideasman42 ah, makes sense re comments.
Also, just tried merging this into asset-engine branch, looks like
-Xours
is mandatory… Should not be an issue with following process though, I think:temp-clang-format
intoasset-engine
(about 6 conflicting files, essentially due to recent pre-clang-format edits in master).temp-clang-format
and commit.asset-engine
and commit.clang-format
intoasset-engine
usinggit merge -Xignore-all-space -Xours temp-clang-format
command.For the final merge, regular merge is catastrophic ( ALL edited files in
asset-engine
are conflicting, over 30). Using only-Xignore-all-space
option, it’s a tad better (although I did not check the actual result), but am still getting 23 files conflicting, even the mere addition of a single function increator.c
is enough to create an issue!Note: changed escaped newlines not to right align:
561e02831c
@ideasman42, the
AlignAfterOpenBracket
differs between the task and actual.clang-format
file.One thing to realize here is that it is not only used by function declarations, but applies for anything in round, square or triangle bracket.
Some examples which i find weird.
With the current .vlang-format, configured to 2 spaces:
Same but indentation is 4 spaces. Better, but still weird"
But this is even more weird:
What is the exact issue of
AlignAfterOpenBracket: Align
? We don't rename functions that often, and if we do the indentation will get fixed by clang-format. A bit more noisy, but most of the noise is cancelled out by ignore-whitespace (-W) flag.Resolved indentation of if-statements by setting
ContinuationIndentWidth: 4
4cf10ef055
Mostly it works fine, but it's quite strange for structs when everything else indents 2 spaces, annoying but acceptable IMHO.
Don't know about template formatting, it does seem a bit odd, but I didn't look into how much control there is over wrapping them.
AlignAfterOpenBracket: Align
Note that w/
ContinuationIndentWidth: 4
, function args now use 4 space indentation - (similar to current convention to use 2x indent width to indent function args). I would have accepted either, but slightly prefer ContinuationIndentWidth to be double indent width because it visually separates indented code blocks from wrapped function args.with.
Template arguments are still acting weird.
Template arguments wrapping is controlled with
AlignAfterOpenBracket
. There is no decoupled setting for this.I would discourage writing such code. But it gets wrapped same way with
Align
policy as well.Counter-example.
AlwaysBreak
policy:Align
policy:The function is very unlikely to be renamed, and with
Align
policy you don't "waste" an entire line.Long story short: i am a strong believer that local readability of code is way more important than potential noise in diffs.
To avoid/reduce noise in diffs you can use
git diff -W
, encourage code review where naming gets addressed.There is nothing you can do with the code which is already in. Which is getting even more depressing when poor decisions are made to compensate for lack of attention somewhere else.
Am strongly against
Align
, the claim of local readability is subjective too.You could for eg have:
With
AlwaysBreak
policy,As for
git diff -W
, this is OK, but we can't always or easily control the conditions for reading diffs (each tool needs a way to configure this ~ I couldn't find a way to do this in diffusion for eg, and even when it's possible - you need to remember to turn it on... in practice it's often a hassle and we just loose mental cycles reading things that didn't change, or we accidentally leave it on and risk not properly reviewing significant whitespace change from Python which could be in the same diff).These changes are also more likely to conflict when merging branches.
It's not just function renaming, return value use may be added/removed.
eg, it's common enough to use buttons only when setting a flag.
Becomes:
1 line declaring a variable becomes 9 lines changed.
I see point, but things are a bit.. complicated. The exact examples you've shown are not correct.
With
AlwaysBreak
:With
Align
:The exact UI code will be formatted like this with
Align
:clang-format will not break list of arguments or parameters if they fit into a single line width after wrapping to the next line.
So you are still "risking" having more lines changed in the diff with either of the options. We can't fully protect against that, and the examples i saw will behave the same with either
Align
orAlwaysBreak
(at the same time mine examples are somewhat subjectively more readable).Well, sure. But here are two sides to this:
Did you try
Align
in an existing code of your areas and check how bad of decisions are actually being made there?Crazy idea, how INSANE would it be to have clang-formats in some modules folders? Like, i know i am working with templates and don't use those insane arguments lists. so what if i just override
AlignAfterOpenBracket
for those modules? :)EDIT: Ignore the code from email, copy-pasted original code for the UI example, should have copy-pasted the formatted one.
Ah, you're right about the exact case with assignment, nevertheless, clang-format will make different wrapping decisions besed on length of arguments, function names and assignment.
Example with
AlwaysBreak
from interface layout (width set to 80 but similar cases exist with wider width allowed)It's mixing indentation styles in a way thats a bit odd (my first point).
re:
This depends, the example I gave would have indented all args were the tooltip shorter,
since exact output depends on the length of the args, meaning you could edit a tooltip and re-indent all function args too.
eg, this is the output with the tip edited
The original point you were making about not renaming arguments often is correct, but it looks like clang-format is going to re-wrap args based on other minor edits, and I'm not convinced that this makes a significant difference in readability, in fact, if it did - you should be concerned that clang-format is adding breaks even in cases when we ask them not to be added (when switching to
Align
).Here are examples of
Align
I'm not a fan of.For any auto-formatting one-size-fits-all, we will be able to find examples that are weak,
just showing some code examples are aren't necessarily more readable.
The diff-noise issue remains too.
If maintainers have strong opinion about a module which isn't sharing much logic w/ the rest of Blender, I don't see why not.
We'd only want this to be used in exceptional cases when there is good reason for it - not just personal preference.
EDIT the example you give w/ templates seems like a good reason :)
That is the point. There are always cases when it happens, and this is something we can not really affect. Some examples.
Before edits:
then you add
FLAG
, and the code is re-formattedOr (slightly modified the actual code to show behavior):
If you make description a bit more detailed:
Also, here is an example which is kind of stupid with
AlwaysBreak
:I wouldn't say it's incorrect behavior. It's kind of more helpful if shorter arguments fits into a single line. Just a trade-off when you add more stuff then things will be re-wrapped,
Personally, i don't see this as a huge deal and prefer the way how clang-format handles this. And also prefer the
Align
policy since that saves quite some lines.Weeeeelll. Technically, you could work with indentation of 2-spaces+single-tab (as in space space tab). Not pleasant, but you could survive. And desire to go to a different indentation is kind of personal preference =/
Big part of difference in preference here seems to be caused by you mainly working in C domain, and me working in C++ domain. In C++ functions are shorter, since they are within a proper namespace. In C the namespace is part of a function name, which adds extra annoyance.
Would need to poke some more code to see different areas, have feeling of them and such. And encourage everyone to do this to see overall happyness factor!
Just so IRC discussion is not getting lost.
The
Align
vs.AlwaysBreak
might be solved by preferring former for C++ and latter for the rest of the code base. This is something i will investigate if it's always the case in C++ code of Blender, or is it just handful of areas which are exception whereAlign
is looking better.Are we going to switch to space indentation in CMake files as well?
Think we could move to 2 spaces in CMake as well (it's even CMake's own convention), we use it for
./build_files/cmake/Modules
already.I've committed some changes to
.clang-format
which we seemed to agree with @brecht.Align
vs.AlwaysBreak
One thing to be aware here is the behavior of clang-format when you are adding/removing arguments. Clang-format will try to keep all the arguments on a single line unless there is no other way around it.
Consider this example:
If it happens so that after adding
argument4
the line exceeds the columns limit, it will be formatted the following way:And only if after the wrapping the arguments do not fit into a line clang-format will start start breaking them and take
AlignAfterOpenBracket
into account.So right here i don't see how the argument for
AlwaysBreak
stands form the diff noise point of view: changing call signature happens more often than function rename.AlwaysBreak
also makes it worse readable code in C++ with templates and methods.To me more readable code with
Align
seems more important than a fear of more noisy diffs due to re-wrapping. The latter one we can not avoid no matter what settings we use. To me it is also important to avoid any possible complexity with setup which was discussed (the separate configuration for C++).Think @brecht also prefers
Align
, but it'd let him speak for himself and quantify his preference level.My preference/importance here goes as:
AlwaysBreak
, try to live with that in C++, and if that becomes an unbareable PITA only then go into multiple.clang-format
setup.Re-running same version of clang-format
There seems to be few places in code where re-running clang-format will modify sources again (simple
make format
thenmake-format
and second one will do some modifications).Probably can be solved with re-formatting in some places and adding extra lines in anothers.
Objective-C
This is where different versions of clang-format will behave different.
Version 6 will not canonically wrap and align arguments by
:
. For example, clang-format-6 will wrap line this way:while more ocmmonly used style and what clang-format-7 and clang-format-8 will do is
Other issue i'm having here is that i can not make clang-format-7 to wrap brace after Objective-C method. Setting
AfterObjCDeclaration
totrue
doesn't seem to help. In fact, i can not make clang-format-7 to wrap the braces at all. So the code always stays likePossible solutions:
AfterFunction
tofalse
, which is like being a rebel and i know that some of developers really prefers brace to be on a new line after a function.The latter one is probably most realistic solution for us.
"Stability" with multiple clang-format versions
Apart from two aspects from above (corner cases with re-running clang-format twice and Objective-C) seems all fine. Didn't see a difference in C/C++ code with different formatter versions, so don't think we have an issue (possibility was that if two developers work on a same file and have different clang-format version the code will be formatted different all the time).
I do indeed prefer
Align
personally.I've committed a change that makes
make format
run multithreaded, now it completes in ~10s for me.Gave it a quick shake on windows:
I need this change on windows to make it run, not sure what is up there.
Process creation is a relatively expensive operation on windows, even multi-threaded this still needs a couple of minutes to run, clang format will happily format multiple files in a single invocation so if we batch them by folder or something we can probably speed this up a little.
another thing is that the exclusions don't seem to work, slashes are pointing the right way and the case matches but somethow sobol.cpp still didn't get excluded and clang-format as expected got stuck on it.
i'll need to give make.bat some love to make this work out of the box, and it will require a local py 3.x install (we can't use the blender one, since we don't know where it lives, build folders are flexible and can't be counted on) and the one from the svn libs is not in a runnable state, no super thrilled about that but it is what it is.
got exclusions to work, git outputs unix style paths even on windows so no conversion is needed.
@LazyDodo, committed your fixes and batching to improve performance. Did not test performance on Windows yet, on Linux here time went from 10s to 6s.
I didn't see any improvement in the batched version, which was curious, moved it to an SSD and ran some benchmarks.
TabExpand was taking about 4.5 of that 28.2 seconds so there may be a second of 2 to be had there by multi-threading that as well, but it's pretty clear the low hanging optimization fruit is gone. (i wouldn't even bother with the tabexpand honestly)
TabExpand will be removed once the clang migration is done, so not worth optimizing.
Why do we need
make format
when formatting many files at once is forbidden?The performance of the current
make format
does not matter then, does it?We will still use make format when updating branches and possibly applying patches, although the ability to only format changed files should work in that case.
These aren't happening every commit so a few extra seconds shouldn't matter much.
Other notes:
I'd rather not maintain both batch and a python version, i may take a stab at putting the core logic into cmake with a small helper batch to invoke it, so at-least the core can be shared among platforms
Regarding
Align
versusAlwaysBreak
, I don’t have any strong opinion here, imho we'll have some 'bad' results in both cases, so… Am feeling neutral here. ;)Changed status from 'Open' to: 'Resolved'
ClangFormat is now in use:
e12c08e8d1
Added subscriber: @MyDeveloperDay
@ideasman42 is this of interest? https://reviews.llvm.org/D68296
@MyDeveloperDay great! although there would be some delay before we would apply this change.
Just reviewing your other concerns with clang-format
I believe if you use
AfterCaseLabel: 'true'
in the BraceWrapping you will get closer to the style in your style guide, this was landed in April of 2019 https://reviews.llvm.org/D52527, given the timing I would expect this to require clang-format 9.0With this setting the example from your style guide formats as follows