Page MenuHome

RFC: Clang-format for Blender
Open, NormalPublic

Description

UPDATED JAN 7/2019 - Ultimately this proposal will do two things

  • Change a few parts of Blender's C/C++ code style; like using spaces instead of tabs, and some other minor things.
  • Incorporate an automated way to enforce this style across Blender.

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

  1. Commit a .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.
  2. Module owners can chose to run clang-format on files they are working on if they want, but running mass format updates is strongly discouraged.
  3. Clang format results should not be blindly applied, and all changes by clang-format should be reviewed carefully.
  4. New contributors are encouraged to use clang-format on their changes, but should not run clang-format on other unrelated parts of their code. At first, this will not work as well since much of the existing Blender code doesn't adhere to its own style guide, but over time as more files are run through clang format, this will become less problematic.
  5. Make a wiki page to describe how to use clang-format, and explaining how NOT to use it (e.g. banning wholesale reformatting of many files, etc).
  6. Over time, more and more files would become "clang-format clean" where running clang-format on the file will produce no changes.
  7. 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.

  1. If you have existing changes in a file you want to work on, either commit/merge the changes first, or stash them.
  2. Run clang-format -i <filename> on the file you are working on.
  3. Using git diff or git integration with your editor, examine the changes introduced by clang-format to make sure they are acceptable.
  4. In cases where clang-format shouldn't be run, then add /* clang-format off */ and /* clang-format on */ as needed, or use // to prevent line merges (see below for details)

Do / Do not

  • DO NOT: Run clang-format on many files and blindly commit them
  • DO NOT: Mix changes from clang-format with other changes (e.g. renaming variables, refactoring a function, etc).
  • DO: Run clang-format on only the files you are working on (at most a handful).
  • DO: Create separate clang-format patches that only update the format to be clang-format clean, with no other changes.

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.

  • Breaking after { for multiline for loops and if statements: clang-format is an all-or-nothing situation for {. There is no way to optionally break before a { if the if or for body is more than one line. Adding this would require making upstream changes to clang-format.
  • Always stacking arguments instead of packing on one-line with a continuation: Currently there is no way to force clang-format to always stack instead of packing.
  • Version check for clang-format: Currently there is no way to enforce a version when running clang-format. Later on we may want to require this since the results of clang-format can sometimes vary with the version.
  • Bug: There is a case where comment intents are wrong right before a #ifdef line. Unclear why.
  • Bug: Wrapping on for loops with long initializations that use , 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

  • Q: What about forks or branches?
  • A: There isn't an easy answer here. Solution is either for branches to merge first, then reformat, or to run clang-format independently on the patches.
  • Q: What about the Blender style requirements that don't work with clang-format yet?
  • A: The most problematic ones are (a) the indent-after # issue with the preprocessor and (b) the newline before { for if 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.
  • Q: What about enforcing non-whitespace changes like requiring {} everywhere even for one-line statements?
  • A: Non-whitespace changes aren't supported by clang-format. However, these type of issues can be corrected with clang-tidy, which is a bit more powerful. That's a something that can get looked at later; for now, this proposal is sticking to just clang-format, since it is more widely available and more stable.
  • Q: What about platform support (e.g. Mac, Windown, Linux)?
  • A: On Mac, clang-format is available by default as part of XCode. On Linux it is available with sudo apt-get install clang-format. On Windows, it is available as part of the Windows builds.

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 to make.bat). Then run clang-format -i <file>.

UPDATE JAN 7/2019: See Campbell's temp-clang-format branch for the latest .clang-format config.

# Copyright 2017 Blender Foundation
#
# Licensed under the Apache License, Version 2.0 (the "License"); you may not
# use this file except in compliance with the License.  You may obtain a copy
# of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
# License for the specific language governing permissions and limitations under
# the License.
#
# Clang-format for Blender, as describe in the Blender style guide:
# https://wiki.blender.org/index.php/Dev:Doc/Code_Style
#
# NOTE: Not all Blender style rules are currently supported properly! Take care
# when using this (see below for details).
#
# To apply clang-format to a file, run
#
#   clang-format -i foo.cpp
#
# This will update the file in place.
#
# NOTE: At time of writing (10/30/2017) not all formatting can be made exactly
# like current Blender sources, so a few compromises are made:
#
#   1. Newline after : in switch statements: clang-format will put the { on
#      the same line. This is due to a limitation in clang-format; it does not
#      support adding the newline after cases in switch statements.
#   2. Nested preprocessor directives don't get proper indentation after the
#      '#'. See IndentPPDirectives, which is supported in clang-format from
#      LLVM6, but not LLVM5. It is included below but commented out.
#   3. Special case of { position on if statements where the condition covers
#      more than one line. clang-format is an all or nothing formatter in this
#      case; so unfortunately the case of
#
#      if (long_condition_here() ||
#          long_condition_here() ||
#          long_condition_here() ||
#          long_condition_here())
#      {
#          ...
#      }
#
#      will become
#
#      if (long_condition_here() ||
#          long_condition_here() ||
#          long_condition_here() ||
#          long_condition_here()) {
#          ...
#      }
#

# Configuration of clang-format
# =============================

# This causes parameters on continuations to stack after the open brace,
# instead of getting wrapped and indented.
#
#   like_this(parameter_one,
#             parameter_two,
#             parameter_three);
#
AlignAfterOpenBracket: Align

# Disallow short functions on one line; break them up.
AllowShortBlocksOnASingleLine: 'false'

# These two settings trigger stacking of parameters in most cases; this is
# easier to read and also makes diffs easier to read (since an added or removed
# parameter is obvious). For example, function calls will look like this:
#
#   like_this(parameter_one,
#             parameter_two,
#             parameter_three,
#             parameter_four,
#             parameter_five,
#             parameter_six);
#
# instead of this
#
#   like_this(parameter_one, parameter_two, parameter_three, parameter_four,
#             parameter_five, parameter_six);
#
BinPackArguments: 'false'
BinPackParameters: 'false'

# 120 is the Blender standard. However, 80 columns is generally preferred.
# Since 120 should be the exception, use a 80-column limit for clang format. If
# this needs to be different, then a developer has two choices: Either manually
# change the result of running clang-format, or introduce '// clang-format off'
# and '// clang format on' markers to disable clang-format for that section.
ColumnLimit: '80'

# Cause initializer lists to have one member initialized per line, in the case
# that all initializers can't fit on a single line.
ConstructorInitializerAllOnOneLineOrOnePerLine: 'true'

# Don't indent the : after a constructor. For example:
#
#   explicit foo_class ()
#   : member1_(5)
#   {
#   }
#
ConstructorInitializerIndentWidth: '0'

# This will unfortunately use spaces in some cases where it's not desired (like
# function calls) but the overall result is better since it will allow
# alignment to work properly with different tab width settings.
ContinuationIndentWidth: '8'

# This tries to match Blender's style as much as possible. One
BreakBeforeBraces: 'Custom'
BraceWrapping: {
    AfterClass: 'true'
    AfterControlStatement: 'false'
    AfterEnum : 'false'
    AfterFunction : 'true'
    AfterNamespace : 'false'
    AfterStruct : 'false'
    AfterUnion : 'false'
    BeforeCatch : 'true'
    BeforeElse : 'true'
    IndentBraces : 'false'
}

# For switch statements, indent the cases.
IndentCaseLabels: 'true'

# TODO: After clang 6.0 is released more broadly, turn this option on. It will
# indent after the hash inside preprocessor directives, as is typically done
# now. Unfortunately for now, this means some preprocessor directives won't be
# formatted quite correctly. However, this is a small price to pay for the
# overall utility of clang-format.
#IndentPPDirectives: 'AfterHash'

SpaceAfterTemplateKeyword: 'false'

# Use "if (...)" instead of "if(...)", but have function calls like foo().
SpaceBeforeParens: ControlStatements
SpaceInEmptyParentheses: 'false'

# Use two spaces before trailing comments, for example
#
#   foo = bar;  /* comment */
#
SpacesBeforeTrailingComments: '2'

# Blender uses tabs for indentation, but assume 4-space tabs.
# Note: TabWidth and IndentWidth must be the same, or strange things happen.
UseTab: 'ForIndentation'
TabWidth: '4'
IndentWidth: '4'

# Add a big penalty on breaking after the return type of functions. For example,
#
#   static void foo(...)
#
# instead of
#
#   static void
#   foo(very long content here that maybe could be stacked)
#
PenaltyReturnTypeOnItsOwnLine: 10000

# There are macros in Blender for custom for loops; tell Clang to treat them
# like loops rather than an expression, and so put the { on the same line.
ForEachMacros: ['BMO_ITER', 'BM_ITER_MESH', 'BM_ITER_ELEM']

Details

Related Objects

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

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

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:

  • 2 spaces indentation (no tabs anywhere)
  • 80 max width

P560

also made other modifications:

  • use 4-spaces for continuations instead of 8 (since the intention here is to use a double-indent).
  • disabled comment flow since re-wrapping comments can loose their intended layout.
  • disable AlignAfterOpenBracket (while nice for short names - causes right-shift and diff-noise when minor changes are made to the function name or a variable that assigns it's result).

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,

uiButLongFunctionName(
        uiblock,
        &(const UIRect){.x = xpos, .y = ypos, .w = UNIT * 2, .h = UNIT},
        flag,
        tooltip);

Of course using this _only_ to avoid splitting arguments over too many lines isn't a good reason to do so.


Remaining issues:

  • Struct declarations have 8 characters, sometimes wrapped strangely.
  • Structs are also wrapped strangely.

before:

struct ApplicationState app_state = {
	.signal = {
		.use_crash_handler = true,
		.use_abort_handler = true,
	},
	.exit_code_on_error = {
		.python = 0,
	}
};

after:

struct ApplicationState app_state = {.signal =
                                             {
                                                     .use_crash_handler = true,
                                                     .use_abort_handler = true,
                                             },
                                     .exit_code_on_error = {
                                             .python = 0,
                                     }};

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):

void BKE_override_property_operation_delete(
        IDOverrideProperty *override_property, IDOverridePropertyOperation *override_property_operation)
{
	bke_override_property_operation_clear(override_property_operation);
	BLI_freelinkN(&override_property->operations, override_property_operation);
}

giving…

void BKE_override_property_operation_delete(
    IDOverrideProperty *override_property,
    IDOverridePropertyOperation *override_property_operation) {
  bke_override_property_operation_clear(override_property_operation);
  BLI_freelinkN(&override_property->operations, override_property_operation);
}

	if (!strict && (subitem_locindex != subitem_defindex) &&
	    (opop = BLI_listbase_bytes_find(&override_property->operations, &subitem_defindex, sizeof(subitem_defindex),
	                                    offsetof(IDOverridePropertyOperation, subitem_local_index))))
	{
		if (r_strict) {
			*r_strict = false;
		}
		return opop;
	}

giving…

if (!strict && (subitem_locindex != subitem_defindex) &&
    (opop = BLI_listbase_bytes_find(
         &override_property->operations, &subitem_defindex,
         sizeof(subitem_defindex),
         offsetof(IDOverridePropertyOperation, subitem_local_index)))) {
  if (r_strict) {
    *r_strict = false;
  }
  return opop;
}

	if (local->override->reference->override && (local->override->reference->tag & LIB_TAG_OVERRIDE_OK) == 0) {
		BKE_override_update(bmain, local->override->reference);
	}

giving…

if (local->override->reference->override &&
    (local->override->reference->tag & LIB_TAG_OVERRIDE_OK) == 0) {
  BKE_override_update(bmain, local->override->reference);
}

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.

@Bastien Montagne (mont29) - agree with your concerns.

  • First example, think clang5 does this correct.
  • Completely agree with your examples of if statements and brace placement. The improvement over indentation of 4 is that they're now not aligned - which was _really_ bad.

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

@Campbell Barton (campbellbarton) 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).

struct ApplicationState app_state = {
        .signal =
                {
                        .use_crash_handler = true,
                        .use_abort_handler = true,
                },
        .exit_code_on_error =
                {
                        .python = 0,
                },
};

I talked to @Campbell Barton (campbellbarton) 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 Mierle (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 @Campbell Barton (campbellbarton) '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?

Looks like we can work aorund the C99 struct alignment issue by setting BreakBeforeBinaryOperators: All

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


Assuming we do no post-processing then. A direct comment on @Campbell Barton (campbellbarton) 's blockers:

  • Brace Placement w/ Multi-Line Checks If we switch to 2-space indentation this problem disappear.

Agreed (although still find braces on newlines more readable in this case).

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

Ok, mainly the UI code gives awkward wrapping, we could use structs more in this case, eg: rcti instead of x,y,w,h.

  • Preprocessor In this case I would say just use /* clang-format off */

This would mean disabling clang-format for most pre-processor heavy blocks.

  • Array-indentation Not sure what is the problem here. The 8 indentation?

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 to IndentWidth.

Some inlined answers

Given 2.8 has finally made it to master, is there any chance we can get this moving again?

Would absolutely love to!

This would mean disabling clang-format for most pre-processor heavy blocks.

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:

  • Disable clang-format there.
  • Make the code more sane.
  • Re-enable clang-format for those areas.

On a bigger scale this is how i see this proposal:

Pros:

  • Lower entry point for new developers to get into code style.
  • Less time spent on adapting code style from incoming patches.
  • More strict definition of the style, less objective factors (once we agree on the final setup of course).
  • Scalability to other areas, like Cycles ;)

Cons:

  • Badly written code becomes more unreadable.

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:

  • Task force during code quest, which pulled a lot of energy adopting those to our style
  • Looking into all the weird and wonderful areas of Blender with their own style, naming conventions and such.
  • Looking into the history of specific code parts, which more often than always ends up chasing a lot of "Code cleanup" commits.
  • Explaining in the IRC go the new comers and other contributors how they should format specific change.

@Campbell Barton (campbellbarton), 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.

Assuming we do no post-processing then. A direct comment on @Campbell Barton (campbellbarton) 's blockers:

  • Brace Placement w/ Multi-Line Checks If we switch to 2-space indentation this problem disappear.

Agreed (although still find braces on newlines more readable in this case).

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 Mierle (keir) @Sergey Sharybin (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…).

Assuming we do no post-processing then. A direct comment on @Campbell Barton (campbellbarton) 's blockers:

  • Brace Placement w/ Multi-Line Checks If we switch to 2-space indentation this problem disappear.

Agreed (although still find braces on newlines more readable in this case).

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!

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.

@Keir Mierle (keir) @Sergey Sharybin (sergey) I totally agree automatic formatting is a great tool to have - as long as it does produce good readable code. ;)

+1

@Campbell Barton (campbellbarton), are you still opposed to this approach? From reading the comments, it appears you are the primary detractor.

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.

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.

Agree with your sentiment here.

Tried tweaking options and here are two which I think are acceptable.

  • Conditional braces always on newline as @Bastien Montagne (mont29) suggested (so no lines running into each other in an unreadable way)
  • We'll need remember to use trailing commas for struct declarations, to avoid formatting bug (as @Dalai Felinto (dfelinto) points out).
  • Continuations are using a single indentation, I don't like this much but continuation indentation is used for structs & arrays - where having double indentation seems quite odd.
  • Large pre-processor blocks that benefit from indentation can just disable clang formatting.

If we keep 4 space indentation I don't think 80 char width limit is reasonable.

Suggest either of these:

  • P884 (4 char indent, 120 width).
  • P885 (2 char indent, 80 width).

Notes:

  • comments and 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).
  • We'll need to add trailing commas to all struct declarations.

+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:

AllowShortFunctionsOnASingleLine: Inline

ForEachMacros should have 'foreach' as used in Cycles.

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

This would be ideal, the issue AFAICS is there is no way to tell clang-format to do this.

Personally I like a wider with, though maybe 100 instead of 120 is a better compromise for side by side code viewing?

100 is fine, wouldn't want lower for 4-space indent though (Rust's convention is <=99, found it OK).

Also think this could be added:

AllowShortFunctionsOnASingleLine: Inline

ForEachMacros should have 'foreach' as used in Cycles.

+1 for both. updated P884 config, although many more foreach macros are needed.

Also committed trailing commas to workaround strange struct indentation rBe305560f13c3c67b048dc3889c7797eae2e345c4

Also +1 for the 4 char version.

ForEachMacros is also missing some from BLI, at least LISTBASE_FOREACH.

The ones with BEGIN/END are not concerned here I guess? Like FOREACH_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 of ForEachMacros have no effect.

Keir Mierle (keir) added a comment.EditedJan 6 2019, 7:18 PM

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

This would be ideal, the issue AFAICS is there is no way to tell clang-format to do this.

Right, clang-format can't add or remove braces. I just meant for the guidelines that we follow manually.

Edit: Since AfterControlStatement: 'true' is set, Values of ForEachMacros have no effect.

It seems to have an effect when there are no braces.

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

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 default All.

This would be ideal, the issue AFAICS is there is no way to tell clang-format to do this.

Right, clang-format can't add or remove braces. I just meant for the guidelines that we follow manually.

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

This would be ideal, the issue AFAICS is there is no way to tell clang-format to do this.

Right, clang-format can't add or remove braces. I just meant for the guidelines that we follow manually.

Edit: Since AfterControlStatement: 'true' is set, Values of ForEachMacros have no effect.

It seems to have an affect when there are no braces.

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:

if ((tangent_mask & DM_TANGENT_MASK_ORCO) && CustomData_get_named_layer_index(loopdata_out, CD_TANGENT, "") == -1)
    CustomData_add_layer_named(loopdata_out, CD_TANGENT, CD_CALLOC, NULL, (int)loopdata_out_len, "");

Becomes:

if ((tangent_mask & DM_TANGENT_MASK_ORCO) &&
    CustomData_get_named_layer_index(loopdata_out, CD_TANGENT, "") == -1)
    CustomData_add_layer_named(
        loopdata_out, CD_TANGENT, CD_CALLOC, NULL, (int)loopdata_out_len, "");

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.

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

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 default All.

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.

  • Update comments in the config.
  • Add 'for' macros to config.
  • Define which parts of the code-base will use this (assume source/ and intern/) - although intern/cycles might be handled in a separate pass.
  • Create scripts to:
    • Initial migration (output of clang-format isn't usable as-is).
      • Check output and disable auto-formatting when its not acceptable (from a quick check at least 10 or so of these cases exist).
      • Tweaks to some parts of the code to make auto-formatting work (again, a handful of cases this is needed, some already handled).
    • Script to run on the entire code-base (for occasional updates, not migration).
    • Script to run on modified files (this will be used most often for regular development).

@Brecht Van Lommel (brecht),

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 default All.

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.

@Campbell Barton (campbellbarton), is just source/ with a selective directories in intern/, 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:
char   vgroup[64];  /* ... long comment... */

into

char   vgroup
    [64];  /* ... long comment... */

Makesdna could support this but we wont want this formatting, suggest to put comments on line above struct member.

Otherwise, next steps.

Let's not forget clear instructions for new and old developers: updated code-style, IDE, and build wiki documents.

This is great! Nice progress.

@Campbell Barton (campbellbarton) - Why the need for the custom replacement of tabs with spaces in Python? clang-format handles that natively.
@Dalai Felinto (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:

struct MySdnaStruct {
    float my_mbr; /* short description */
    float my_ot_mbr; /* too short description, name */
}

Suggested:

struct MySdnaStruct {
    /* Not so short description; has all the details! Also, by having
     * the variable on a line by itself, longer and more descriptive variable
     * names are encouraged. */
    float my_member;

    /* Another not too short description. Includes descriptions of important
     * preconditions or caveats about this particular SDNA member. */
    float my_other_member;
}

@Keir Mierle (keir) moved comments above struct members (preferred this anyway, multi-line comments became annoying to wrap and properly indent). rB5a43406e1bad973a8cb32702b4fdb588068a6dcd

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

A: On Mac, clang-format is available by default as part of XCode. On Linux it is available with sudo apt-get install clang-format. On Windows, it is available as part of the Windows builds.

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 (LazyDodo) Good catch on the .editconfig discrepancy. I've extended T60279 to include updating other IDE configs that exist in the Blender source.

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 Sharybin (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 in temp-clang-format.

  • Updated .editorconfig
  • Added intern/ghost, intern/clog

Some other points:

  • Would be nice to go tabs -> spaces everywhere (the biggest change will be CMake files).

    Outside scope of this task strictly speaking, but would be nice to simplify things so developers don't need to configure there editor with different tab/space indentation per file type.
  • Assume generated code will switch to spaces too - see makesrna.c, clang-format wont handle this so needs to be done manually.

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.

#define __func__ __FUNCTION__

Applies to:

  • ./intern/clog/CLG_log.h
  • ./intern/guardedalloc/intern/mallocn_intern.h
  • ./source/blender/blenlib/BLI_compiler_compat.h

I've added most intern/ modules for formatting now in temp-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 Van Lommel (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?

[1] https://github.com/llvm-mirror/clang/blob/master/tools/clang-format/git-clang-format

Added tests/gtest, removed BLI_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.

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?

[1] https://github.com/llvm-mirror/clang/blob/master/tools/clang-format/git-clang-format

+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:

  • Added FixNamespaceComments ~ seems generally helpful.
  • Use clang-format off instead of excluding files (except for sobol.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:

DefNode(
    ShaderNode,
    SH_NODE_BSDF_ANISOTROPIC,
    def_anisotropic,
    "BSDF_ANISOTROPIC",
    BsdfAnisotropic,
    "Anisotropic BSDF",
    "");
DefNode(ShaderNode, SH_NODE_BSDF_DIFFUSE, 0, "BSDF_DIFFUSE", BsdfDiffuse, "Diffuse BSDF", "");
DefNode(
    ShaderNode,
    SH_NODE_BSDF_PRINCIPLED,
    def_principled,
    "BSDF_PRINCIPLED",
    BsdfPrincipled,
    "Principled BSDF",
    "");
DefNode(ShaderNode, SH_NODE_BSDF_GLOSSY, def_glossy, "BSDF_GLOSSY", BsdfGlossy, "Glossy BSDF", "");
DefNode(ShaderNode, SH_NODE_BSDF_GLASS, def_glass, "BSDF_GLASS", BsdfGlass, "Glass BSDF", "");
DefNode(
    ShaderNode,
    SH_NODE_BSDF_REFRACTION,
    def_refraction,
    "BSDF_REFRACTION",
    BsdfRefraction,
    "Refraction BSDF",
    "");

There are also many comments at the ends of lines that cause strange formatting.

Eg:

#define TD_BEZTRIPLE (1 << 12) /* long comment */

Becomes:

#define TD_BEZTRIPLE                                                                              \
    (1                                                                                            \
     << 12) /* long comment */

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 the temp-clang-format branch and check the results are acceptable so we can move forward with this task.

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.

#define USE_WORLD_ORIENTATION(wpd) ((wpd->shading.flag & V3D_SHADING_WORLD_ORIENTATION) != 0)
#define STUDIOLIGHT_TYPE_WORLD_ENABLED(wpd)                                                       \
    (STUDIOLIGHT_ENABLED(wpd) && (wpd->studio_light->flag & STUDIOLIGHT_TYPE_WORLD))
#define STUDIOLIGHT_TYPE_STUDIO_ENABLED(wpd)                                                      \
    (STUDIOLIGHT_ENABLED(wpd) && (wpd->studio_light->flag & STUDIOLIGHT_TYPE_STUDIO))
#define STUDIOLIGHT_TYPE_MATCAP_ENABLED(wpd)                                                      \
    (MATCAP_ENABLED(wpd) && (wpd->studio_light->flag & STUDIOLIGHT_TYPE_MATCAP))
#define SSAO_ENABLED(wpd)                                                                         \
    ((wpd->shading.flag & V3D_SHADING_CAVITY) &&                                                  \
     ((wpd->shading.cavity_type == V3D_SHADING_CAVITY_SSAO) ||                                    \
      (wpd->shading.cavity_type == V3D_SHADING_CAVITY_BOTH)))
#define CURVATURE_ENABLED(wpd)                                                                    \
    ((wpd->shading.flag & V3D_SHADING_CAVITY) &&                                                  \
     ((wpd->shading.cavity_type == V3D_SHADING_CAVITY_CURVATURE) ||                               \
      (wpd->shading.cavity_type == V3D_SHADING_CAVITY_BOTH)))

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?

@Bastien Montagne (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.

@Campbell Barton (campbellbarton) 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:

  1. Merge temp-clang-format into asset-engine (about 6 conflicting files, essentially due to recent pre-clang-format edits in master).
  2. Apply clang-format on temp-clang-format and commit.
  3. Apply clang-format on asset-engine and commit.
  4. Merge clang-format into asset-engine using git 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 in creator.c is enough to create an issue!

Note: changed escaped newlines not to right align:

Test not aligning escaped newlines

They're annoying to edit if you ever dont have auto-formatting enabled
and with 100 char width, they may run off the screen.

rB561e02831c5501d883dba8214f55e32d36628b0c

@Campbell Barton (campbellbarton), 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:

if (
  psmd->mesh_final->totvert != psmd->totdmvert || psmd->mesh_final->totedge != psmd->totdmedge ||
  psmd->mesh_final->totface != psmd->totdmface) {
  psys->recalc |= ID_RECALC_PSYS_RESET;

  psmd->totdmvert = psmd->mesh_final->totvert;
  psmd->totdmedge = psmd->mesh_final->totedge;
  psmd->totdmface = psmd->mesh_final->totface;
}

Same but indentation is 4 spaces. Better, but still weird"

if (
  psmd->mesh_final->totvert != psmd->totdmvert ||
  psmd->mesh_final->totedge != psmd->totdmedge ||
  psmd->mesh_final->totface != psmd->totdmface) {
    psys->recalc |= ID_RECALC_PSYS_RESET;

    psmd->totdmvert = psmd->mesh_final->totvert;
    psmd->totdmedge = psmd->mesh_final->totedge;
    psmd->totdmface = psmd->mesh_final->totface;
}

But this is even more weird:

template<
  typename EVAL_VERTEX_BUFFER,
  typename STENCIL_TABLE,
  typename PATCH_TABLE,
  typename EVALUATOR,
  typename DEVICE_CONTEXT = void>
class FaceVaryingVolatileEval
{
};

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
rB4cf10ef0557255428ba6563c5d7cd5195778e024

Mostly it works fine, but it's quite strange for structs when everything else indents 2 spaces, annoying but acceptable IMHO.

struct ApplicationState app_state = {
    .signal =
        {
            .use_crash_handler = true,
            .use_abort_handler = true,
        },
    .exit_code_on_error =
        {
            .python = 0,
        },
};

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

  • causes noisy diffs when functions are renamed (even if it's rare) especially because callers will re indent args too.
  • causes noisy diffs if functions become static, return types const.
  • causes right shift on long function names.
  • causes 2x different kinds of wrapping if you have some *very* long function names.
  • nicer for inline comments, eg:
some_function_call_with_long_name_and_many_args(
    some_arg,
    /* Descriptive text, sometimes useful to include here. */
    (const char *[]){"a", "b", "c", NULL},
    /* Descriptive text can even be long and we don't have to worry about over wrapping :) */
    another_arg);

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.

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.

Template arguments wrapping is controlled with AlignAfterOpenBracket. There is no decoupled setting for this.

some_function_call_with_long_name_and_many_args(
   some_arg,
   /* Descriptive text, sometimes useful to include here. */
   (const char *[]){"a", "b", "c", NULL},
   /* Descriptive text can even be long and we don't have to worry about over wrapping :) */
   another_arg);

I would discourage writing such code. But it gets wrapped same way with Align policy as well.


Counter-example.

AlwaysBreak policy:

add_relation(
    texture_key,
    particle_settings_reset_key,
    "Particle Texture",
    RELATION_FLAG_FLUSH_USER_EDIT_ONLY);

Align policy:

add_relation(texture_key,
             particle_settings_reset_key,
             "Particle Texture",
             RELATION_FLAG_FLUSH_USER_EDIT_ONLY);

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:

some_function(some_var
              other_var,
              third_var);
const SomeStruct *variable_name = some_function(some_different_var
                                                other_different_var,
                                                third_different_var);

With AlwaysBreak policy,

some_function(
    some_var
    other_var,
    third_var);
const SomeStruct *variable_name = some_function(
    some_different_var
    other_different_var,
    third_different_var);

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.

uiDefButO_ptr(block,
              UI_BTYPE_BUT,
              ot,
              WM_OP_EXEC_DEFAULT,
              "Copy",
              UI_UNIT_X * 5,
              yco,
              UI_UNIT_X * 5,
              UI_UNIT_Y,
              TIP_("Copy active vertex to other selected vertices (if affected groups are unlocked)"));

Becomes:

uiBut *but = uiDefButO_ptr(block,
                           UI_BTYPE_BUT,
                           ot,
                           WM_OP_EXEC_DEFAULT,
                           "Copy",
                           UI_UNIT_X * 5,
                           yco,
                           UI_UNIT_X * 5,
                           UI_UNIT_Y,
                           TIP_("Copy active vertex to other selected vertices (if affected groups are unlocked)"));
UI_but_flag_enable(but, UI_BUT_DISABLED);

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:

some_function(some_var other_var, third_var);
const SomeStruct *variable_name =
    some_function(some_different_var other_different_var, third_different_var);

With Align:

some_function(some_var other_var, third_var);
const SomeStruct *variable_name =
    some_function(some_different_var other_different_var, third_different_var);

The exact UI code will be formatted like this with Align:

uiBut *but = uiDefButO_ptr(
    block,
    UI_BTYPE_BUT,
    ot,
    WM_OP_EXEC_DEFAULT,
    "Copy",
    UI_UNIT_X * 5,
    yco,
    UI_UNIT_X * 5,
    UI_UNIT_Y,
    TIP_("Copy active vertex to other selected vertices (if affected groups are unlocked)"));
UI_but_flag_enable(but, UI_BUT_DISABLED);

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 or AlwaysBreak (at the same time mine examples are somewhat subjectively more readable).

The claim of local readability is subjective too.

Well, sure. But here are two sides to this:

  • Legitimate code which does get formatted worse all the time, due to options which are..
  • trying to deal with some weird situations, and which will fail in 50% of cases and which does not fully follow Campbell's expectations.

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)

but = uiDefAutoButR(
    block, ptr, prop, index, "", icon, x, y, prop_but_width - UI_UNIT_X, h);

/* BUTTONS_OT_file_browse calls UI_context_active_but_prop_get_filebrowser */
uiDefIconButO(block,
              UI_BTYPE_BUT,
              subtype == PROP_DIRPATH ? "BUTTONS_OT_directory_browse" :
                                        "BUTTONS_OT_file_browse",
              WM_OP_INVOKE_DEFAULT,
              ICON_FILEBROWSER,
              x,
              y,
              UI_UNIT_X,
              h,
              NULL);

It's mixing indentation styles in a way thats a bit odd (my first point).


re:

The exact UI code will be formatted like this with Align

uiBut *but = uiDefButO_ptr(
    block,
    UI_BTYPE_BUT,
    ot,
    WM_OP_EXEC_DEFAULT,
    "Copy",
    UI_UNIT_X * 5,
    yco,
    UI_UNIT_X * 5,
    UI_UNIT_Y,
    TIP_("Copy active vertex to other selected vertices (if affected groups are unlocked)"));
UI_but_flag_enable(but, UI_BUT_DISABLED);

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

but = uiDefButO_ptr(block,
                    UI_BTYPE_BUT,
                    ot,
                    WM_OP_EXEC_DEFAULT,
                    "Copy",
                    UI_UNIT_X * 5,
                    yco,
                    UI_UNIT_X * 5,
                    UI_UNIT_Y,
                    TIP_("Copy active vertex to other selected vertices"));

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


The claim of local readability is subjective too.

Well, sure. But here are two sides to this:

  • Legitimate code which does get formatted worse all the time, due to options which are..
  • trying to deal with some weird situations, and which will fail in 50% of cases and which does not fully follow Campbell's expectations.

    Did you try Align in an existing code of your areas and check how bad of decisions are actually being made there?

Here are examples of Align I'm not a fan of.

void WM_event_set_keymap_handler_callback(wmEventHandler *handler,
                                          void(keymap_tag)(wmKeyMap *keymap,
                                                           wmKeyMapItem *kmi,
                                                           void *user_data),
                                          void *user_data)
{
  handler->keymap_callback.handle_post_fn = keymap_tag;
  handler->keymap_callback.user_data = user_data;
}
if (pyrna_struct_keyframe_parse(&self->ptr,
                                args,
                                kw,
                                "s|ifsO!:bpy_struct.keyframe_insert()",
                                "bpy_struct.keyframe_insert()",
                                &path_full,
                                &index,
                                &cfra,
                                &group_name,
                                &options) == -1) {
  return NULL;
}
static void toolsystem_reinit_with_toolref(bContext *C,
                                           WorkSpace *UNUSED(workspace),
                                           bToolRef *tref);
static bToolRef *toolsystem_reinit_ensure_toolref(bContext *C,
                                                  WorkSpace *workspace,
                                                  const bToolKey *tkey,
                                                  const char *default_tool);
static void toolsystem_refresh_screen_from_active_tool(Main *bmain,
                                                       WorkSpace *workspace,
                                                       bToolRef *tref);
PyDoc_STRVAR(bpy_app_icons_new_triangles_from_file_doc,
             ".. function:: new_triangles_from_file(filename)"
             "\n"
             "   Create a new icon from triangle geometry.\n"
             "\n"
             "   :arg filename: File path.\n"
             "   :type filename: string.\n"
             "   :return: Unique icon value (pass to interface ``icon_value`` "
             "argument).\n"
             "   :rtype: int\n");
static PyObject *bpy_app_icons_new_triangles_from_file(PyObject *UNUSED(self),
                                                       PyObject *args,
                                                       PyObject *kw)
{
if (params.space_type_str && pyrna_enum_value_from_id(rna_enum_space_type_items,
                                                      params.space_type_str,
                                                      &params.space_type,
                                                      error_prefix) == -1) {
  return NULL;
}
else if (params.region_type_str && pyrna_enum_value_from_id(rna_enum_region_type_items,
                                                            params.region_type_str,
                                                            &params.region_type,
                                                            error_prefix) == -1) {
  return NULL;
}
if (base_active && (base_pose == base_active)) {
  bases = BKE_view_layer_array_from_bases_in_mode(view_layer,
                                                  v3d,
                                                  r_bases_len,
                                                  {
                                                      .object_mode = OB_MODE_POSE,
                                                      .no_dup_data = unique,
                                                  });
}

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.

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? :)

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 :)

It looks like clang-format is going to re-wrap args based on other minor edits

That is the point. There are always cases when it happens, and this is something we can not really affect. Some examples.

Before edits:

add_relation(geometry_init_key, point_cache_key, "Geometry Init -> Point Cache");

then you add FLAG, and the code is re-formatted

add_relation(
    geometry_init_key,
    point_cache_key,
    "Geometry Init -> Point Cache",
    RELATION_FLAG_FLUSH_USER_EDIT_ONLY);

Or (slightly modified the actual code to show behavior):

add_relation(
    cow_key, anim_data_key, "Animation CoW -> Animation", RELATION_CHECK_BEFORE_ADD);

If you make description a bit more detailed:

add_relation(
    cow_key,
    anim_data_key,
    "Animation Copy On Write -> Animation",
    RELATION_CHECK_BEFORE_ADD);

Also, here is an example which is kind of stupid with AlwaysBreak:

fprintf(
    stderr,
    "find_node component: Could not find ID %s\n",
    (key.id != NULL) ? key.id->name : "<null>");

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.


We'd only want this to be used in exceptional cases when there is good reason for it - not just personal preference.

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 where Align 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 Van Lommel (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:

some_function_call(argument1, argument2, argument3);

If it happens so that after adding argument4 the line exceeds the columns limit, it will be formatted the following way:

some_function_call(
    argument1, argument2, argument3, argument4);

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 Van Lommel (brecht) also prefers Align, but it'd let him speak for himself and quantify his preference level.

My preference/importance here goes as:

  • Make sure everyone is aware of specific behavior of clang-format.
  • Try to see how many of core developers in every camp.
  • If majority strongly wants 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 then make-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:

m_openGLContext =
    [[NSOpenGLContext alloc] initWithFormat:pixelFormat shareContext:s_sharedOpenGLContext];

while more ocmmonly used style and what clang-format-7 and clang-format-8 will do is

m_openGLContext = [[NSOpenGLContext alloc] initWithFormat:pixelFormat
                                             shareContext:s_sharedOpenGLContext];

Other issue i'm having here is that i can not make clang-format-7 to wrap brace after Objective-C method. Setting AfterObjCDeclaration to true 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 like

- (void)windowWillEnterFullScreen:(NSNotification *)notification {
  associatedWindow->setImmediateDraw(true);
}

Possible solutions:

  • Ignore Objective-C code for now. Will be annoying, since we are still maintaining those. Also, will make it more complicated to integrate formatting into IDEs, not sure they allows filtering over which source files are covered by automated format and which are not.
  • Set AfterFunction to false, 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.
  • Require everyone who works on Objective-C code to use clang-format-8. it's only handful of active people there, so probably not so bad. This is fragile policy, but clang-format-8 will soon(ish) become wildly spread and available.

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.

diff --git a/clang-format-paths.py b/clang-format-paths.py
index 15e1777bc1e..c3dbabab736 100755
--- a/clang-format-paths.py
+++ b/clang-format-paths.py
@@ -98,7 +98,7 @@ def clang_format_version():
 
 def clang_format_file(f):
     cmd = (
-        CLANG_FORMAT_CMD, "-i", "-verbose", f.encode("ascii")
+        CLANG_FORMAT_CMD, "-i", "-verbose", f
     )
     return subprocess.check_output(cmd, stderr=subprocess.STDOUT)

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.

 clang-format-paths.py | 2 --
 1 file changed, 2 deletions(-)

diff --git a/clang-format-paths.py b/clang-format-paths.py
index 15e1777bc1e..fc8e1ee0f64 100755
--- a/clang-format-paths.py
+++ b/clang-format-paths.py
@@ -48,8 +48,6 @@ extensions = (
 ignore_files = {
     "intern/cycles/render/sobol.cpp",  # Too heavy for clang-format
 }
-if os.sep != "/":
-    ignore_files = set(f.replace("/", os.sep) for f in ignore_files)
 
 print("Operating on:")
 for p in paths:

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

(time in seconds)SSDSpinner
Single Threaded154.25237.17
MultiT hreaded43.18125.06
MultiThreaded Batched28.2126.09

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:

  • We might want the ability to force single threaded since clang-format's memory usage explodes on certain files (see if it becomes a problem).
  • For windows we could have a batch script instead of Python, to avoid depending on the systems Python3.

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