RFC: Clang-format for Blender #53211

Closed
opened 5 years ago by Keir · 101 comments
Keir commented 5 years ago
Collaborator

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

  • 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.
  • 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.
  • Clang format results should not be blindly applied, and all changes by clang-format should be reviewed carefully.
  • 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.
  • 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).
  • Over time, more and more files would become "clang-format clean" where running clang-format on the file will produce no changes.

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.

  • If you have existing changes in a file you want to work on, either commit/merge the changes first, or stash them.
  • Run clang-format -i <filename> on the file you are working on.
  • Using git diff or git integration with your editor, examine the changes introduced by clang-format to make sure they are acceptable.
  • 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](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 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']```
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** - 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. - 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. - Clang format results should not be blindly applied, and all changes by clang-format should be reviewed carefully. - 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. - 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). - Over time, more and more files would become "clang-format clean" where running clang-format on the file will produce no changes. # 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. - If you have existing changes in a file you want to work on, either commit/merge the changes first, or stash them. - Run `clang-format -i <filename>` on the file you are working on. - Using `git diff` or git integration with your editor, examine the changes introduced by clang-format to make sure they are acceptable. - 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](https://llvm.org/builds/ LLVM nightly). **Editor integration** - Emacs: http://clang.llvm.org/docs/ClangFormat.html - SublimeText: https://github.com/rosshemsley/SublimeClangFormat - Vim: https://github.com/rhysd/vim-clang-format - Visual Studio: http://llvm.org/builds/ - Xcode: https://github.com/travisjeffery/ClangFormat-Xcode **Config** The [configuration options are documented at the LLVM site](https://clang.llvm.org/docs/ClangFormatStyleOptions.html). 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']```
Keir commented 5 years ago
Poster
Collaborator

Changed status to: 'Open'

Changed status to: 'Open'
Keir commented 5 years ago
Poster
Collaborator

Added subscriber: @Keir

Added subscriber: @Keir
Sergey commented 5 years ago
Owner
Added subscribers: @mont29, @ideasman42, @brecht, @dfelinto, @Sergey, @fclem
Sergey commented 5 years ago
Owner

@Keir, some questions:

  • Why not to attach the config as a file? :)
  • Is the thing cross platform enough? What are the packages to be installed on Linux, macOS? Does it work on Windows?

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.

@Keir, some questions: - Why not to attach the config as a file? :) - Is the thing cross platform enough? What are the packages to be installed on Linux, macOS? Does it work on Windows? 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.
brecht commented 5 years ago
Owner

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.

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](https://developer.blender.org/diffusion/BL/browse/trunk/lib/win64_vc12/llvm/bin/clang-format.exe), 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.
Keir commented 5 years ago
Poster
Collaborator

Try brew install llvm; that should get clang-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.

Try `brew install llvm`; that should get `clang-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.
Owner

Think its worth considering this, although I have concerns.

  • First question, do developers want this, will it save us time and make our life easier? :) - For patches/branches/contributions - yes. For existing code - think overall result may be slightly worse in many cases - though this is personal preference.
  • Could we use clang-format on a per-module basis? Instead of applying in source/ (maybe move towards applying globally, but at least not to begin with).
  • How much should we accept changing our style to fit in with what clang-format supports?
  • There are times we would want to disable this, I suppose module owners can choose when? (if this is on average more than once per file... it would be cause for concern - but imagine its much less than this).
  • Some blocks of code are written for nice alignment and will read slightly worse with auto-formatting but probably not worth disabling auto-formatting. My main concern is we have slightly less readable code - because its too much hassle to add in comments. (examples below).
  • 80 line length is quite a big change, would rather skip this for now. Its important but we can discuss separately.
  • Aligning arguments with the end of the functions causes a lot of right-shift for long function names. eg:

this

static bool ui_but_is_mouse_over_icon_extra(const ARegion *region, uiBut *but, const int mouse_xy[2])

becomes

static bool ui_but_is_mouse_over_icon_extra(const ARegion *region,
                                            uiBut *but,
                                            const int mouse_xy[2])

this

static void feather_bucket_check_intersect(
        float (*feather_points)[2], int tot_feather_point, FeatherEdgesBucket *bucket,
        int cur_a, int cur_b)

becomes

static void feather_bucket_check_intersect(float (*feather_points)[2],
                                           int tot_feather_point,
                                           FeatherEdgesBucket *bucket,
                                           int cur_a,
                                           int cur_b)
  • From looking at output finding the changes cause too much right shift and use more vertical space than I'd like.
  • Bugs in 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:

	const bool use_outset          = BMO_slot_bool_get(op->slots_in, "use_outset");
	const bool use_boundary        = BMO_slot_bool_get(op->slots_in, "use_boundary") && (use_outset == false);
	const bool use_even_offset     = BMO_slot_bool_get(op->slots_in, "use_even_offset");
	const bool use_even_boundary   = use_even_offset; /* could make own option */
	const bool use_relative_offset = BMO_slot_bool_get(op->slots_in, "use_relative_offset");
	const bool use_edge_rail       = BMO_slot_bool_get(op->slots_in, "use_edge_rail");
	const bool use_interpolate     = BMO_slot_bool_get(op->slots_in, "use_interpolate");
	const float thickness          = BMO_slot_float_get(op->slots_in, "thickness");
	const float depth              = BMO_slot_float_get(op->slots_in, "depth");

after...

	const bool use_outset = BMO_slot_bool_get(op->slots_in, "use_outset");
	const bool use_boundary = BMO_slot_bool_get(op->slots_in, "use_boundary") &&
	                          (use_outset == false);
	const bool use_even_offset =
	        BMO_slot_bool_get(op->slots_in, "use_even_offset");
	const bool use_even_boundary = use_even_offset; /* could make own option */
	const bool use_relative_offset =
	        BMO_slot_bool_get(op->slots_in, "use_relative_offset");
	const bool use_edge_rail = BMO_slot_bool_get(op->slots_in, "use_edge_rail");
	const bool use_interpolate =
	        BMO_slot_bool_get(op->slots_in, "use_interpolate");
	const float thickness = BMO_slot_float_get(op->slots_in, "thickness");
	const float depth = BMO_slot_float_get(op->slots_in, "depth");

before

	/* use the largest angle */
	mul_v3_fl(tvec,
	          shell_v3v3_normalized_to_dist(tvec,
	                                        len_squared_v3v3(tvec, e_info_a->no) >
	                                        len_squared_v3v3(tvec, e_info_b->no) ?
	                                            e_info_a->no : e_info_b->no));

after

	mul_v3_fl(
	        tvec,
	        shell_v3v3_normalized_to_dist(
	                tvec,
	                len_squared_v3v3(
	                        tvec,
	                        e_info_a->no) >
	                                len_squared_v3v3(
	                                        tvec,
	                                        e_info_b->no)
	                        ? e_info_a->no
	                        : e_info_b->no));

before

	bm_loop_customdata_merge(
	        bm, e_connect,
	        l_a,       BM_edge_other_loop(e_connect, l_a),
	        l_a->prev, BM_edge_other_loop(e_connect, l_a->prev));

after

	bm_loop_customdata_merge(
	        bm,
	        e_connect,
	        l_a,
	        BM_edge_other_loop(e_connect, l_a),
	        l_a->prev,
	        BM_edge_other_loop(e_connect, l_a->prev));

interface_handlers.c

before

		else if ((!ELEM(event->type, MOUSEMOVE, WHEELUPMOUSE, WHEELDOWNMOUSE, MOUSEPAN)) && ISMOUSE(event->type)) {
			if (!ui_but_contains_point_px(but->active->region, but, event->x, event->y)) {
				but = NULL;
			}
		}

after

		else if ((!ELEM(event->type,
		                MOUSEMOVE,
		                WHEELUPMOUSE,
		                WHEELDOWNMOUSE,
		                MOUSEPAN)) &&
		         ISMOUSE(event->type)) {
			if (!ui_but_contains_point_px(
			            but->active->region, but, event->x, event->y)) {
				but = NULL;
			}
		}

before:

			if (ISKEYBOARD(kmi->type)) {
#if 0			/* would rather use a block but, but gets weirdly positioned... */
				uiDefBlockBut(block, menu_change_shortcut, but, "Change Shortcut",
				              0, 0, uiLayoutGetWidth(layout), UI_UNIT_Y, "");
#endif

				but2 = uiDefIconTextBut(block, UI_BTYPE_BUT, 0, ICON_HAND,
				                        CTX_IFACE_(BLT_I18NCONTEXT_OPERATOR_DEFAULT, "Change Shortcut"),
				                        0, 0, w, UI_UNIT_Y, NULL, 0, 0, 0, 0, "");
				UI_but_func_set(but2, popup_change_shortcut_func, but, NULL);

				but2 = uiDefIconTextBut(block, UI_BTYPE_BUT, 0, ICON_NONE,
				                        CTX_IFACE_(BLT_I18NCONTEXT_OPERATOR_DEFAULT, "Remove Shortcut"),
				                        0, 0, w, UI_UNIT_Y, NULL, 0, 0, 0, 0, "");
				UI_but_func_set(but2, remove_shortcut_func, but, NULL);
			}
			else {
				but2 = uiDefIconTextBut(block, UI_BTYPE_BUT, 0, ICON_HAND, IFACE_("Non-Keyboard Shortcut"),
				                        0, 0, w, UI_UNIT_Y, NULL, 0, 0, 0, 0,
				                        TIP_("Only keyboard shortcuts can be edited that way, "
				                             "please use User Preferences otherwise"));
				UI_but_flag_enable(but2, UI_BUT_DISABLED);
			}
		}
		/* only show 'add' if there's a suitable key map for it to go in */
		else if (WM_keymap_guess_opname(C, but->optype->idname)) {
			but2 = uiDefIconTextBut(block, UI_BTYPE_BUT, 0, ICON_HAND,
			                        CTX_IFACE_(BLT_I18NCONTEXT_OPERATOR_DEFAULT, "Add Shortcut"),
			                        0, 0, w, UI_UNIT_Y, NULL, 0, 0, 0, 0, "");
			UI_but_func_set(but2, popup_add_shortcut_func, but, NULL);
		}

after

			but2 = uiDefIconTextBut(
			        block,
			        UI_BTYPE_BUT,
			        0,
			        ICON_HAND,
			        CTX_IFACE_(BLT_I18NCONTEXT_OPERATOR_DEFAULT,
			                   "Change Shortcut"),
			        0,
			        0,
			        w,
			        UI_UNIT_Y,
			        NULL,
			        0,
			        0,
			        0,
			        0,
			        "");
			UI_but_func_set(but2, popup_change_shortcut_func, but, NULL);

			but2 = uiDefIconTextBut(
			        block,
			        UI_BTYPE_BUT,
			        0,
			        ICON_NONE,
			        CTX_IFACE_(BLT_I18NCONTEXT_OPERATOR_DEFAULT,
			                   "Remove Shortcut"),
			        0,
			        0,
			        w,
			        UI_UNIT_Y,
			        NULL,
			        0,
			        0,
			        0,
			        0,
			        "");
			UI_but_func_set(but2, remove_shortcut_func, but, NULL);
		}
		else {
			but2 = uiDefIconTextBut(
			        block,
			        UI_BTYPE_BUT,
			        0,
			        ICON_HAND,
			        IFACE_("Non-Keyboard Shortcut"),
			        0,
			        0,
			        w,
			        UI_UNIT_Y,
			        NULL,
			        0,
			        0,
			        0,
			        0,
			        TIP_("Only keyboard shortcuts can be edited that way, "
			             "please use User Preferences otherwise"));
			UI_but_flag_enable(but2, UI_BUT_DISABLED);
		}
	}
	/* only show 'add' if there's a suitable key map for it to go in */
	else if (WM_keymap_guess_opname(C, but->optype->idname)) {
		but2 = uiDefIconTextBut(block,
		                        UI_BTYPE_BUT,
		                        0,
		                        ICON_HAND,
		                        CTX_IFACE_(BLT_I18NCONTEXT_OPERATOR_DEFAULT,
		                                   "Add Shortcut"),
		                        0,
		                        0,
		                        w,
		                        UI_UNIT_Y,
		                        NULL,
		                        0,
		                        0,
		                        0,
		                        0,
		                        "");
		UI_but_func_set(but2, popup_add_shortcut_func, but, NULL);
	}

Think its worth considering this, although I have concerns. - First question, do developers want this, will it save us time and make our life easier? :) - For patches/branches/contributions - yes. For existing code - think overall result may be slightly worse in many cases - though this is personal preference. - Could we use clang-format on a per-module basis? Instead of applying in `source/` (maybe move towards applying globally, but at least not to begin with). - How much should we accept changing our style to fit in with what clang-format supports? - There are times we would want to disable this, I suppose module owners can choose when? *(if this is on average more than once per file... it would be cause for concern - but imagine its much less than this).* - Some blocks of code are written for nice alignment and will read *slightly* worse with auto-formatting but probably not worth disabling auto-formatting. My main concern is we have slightly less readable code - because its too much hassle to add in comments. (examples below). - 80 line length is quite a big change, would rather skip this for now. Its important but we can discuss separately. - Aligning arguments with the end of the functions causes a lot of right-shift for long function names. eg: this ``` static bool ui_but_is_mouse_over_icon_extra(const ARegion *region, uiBut *but, const int mouse_xy[2]) ``` becomes ``` static bool ui_but_is_mouse_over_icon_extra(const ARegion *region, uiBut *but, const int mouse_xy[2]) ``` this ``` static void feather_bucket_check_intersect( float (*feather_points)[2], int tot_feather_point, FeatherEdgesBucket *bucket, int cur_a, int cur_b) ``` becomes ``` static void feather_bucket_check_intersect(float (*feather_points)[2], int tot_feather_point, FeatherEdgesBucket *bucket, int cur_a, int cur_b) ``` - From looking at output finding the changes cause too much right shift and use more vertical space than I'd like. - Bugs in `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: ``` const bool use_outset = BMO_slot_bool_get(op->slots_in, "use_outset"); const bool use_boundary = BMO_slot_bool_get(op->slots_in, "use_boundary") && (use_outset == false); const bool use_even_offset = BMO_slot_bool_get(op->slots_in, "use_even_offset"); const bool use_even_boundary = use_even_offset; /* could make own option */ const bool use_relative_offset = BMO_slot_bool_get(op->slots_in, "use_relative_offset"); const bool use_edge_rail = BMO_slot_bool_get(op->slots_in, "use_edge_rail"); const bool use_interpolate = BMO_slot_bool_get(op->slots_in, "use_interpolate"); const float thickness = BMO_slot_float_get(op->slots_in, "thickness"); const float depth = BMO_slot_float_get(op->slots_in, "depth"); ``` after... ``` const bool use_outset = BMO_slot_bool_get(op->slots_in, "use_outset"); const bool use_boundary = BMO_slot_bool_get(op->slots_in, "use_boundary") && (use_outset == false); const bool use_even_offset = BMO_slot_bool_get(op->slots_in, "use_even_offset"); const bool use_even_boundary = use_even_offset; /* could make own option */ const bool use_relative_offset = BMO_slot_bool_get(op->slots_in, "use_relative_offset"); const bool use_edge_rail = BMO_slot_bool_get(op->slots_in, "use_edge_rail"); const bool use_interpolate = BMO_slot_bool_get(op->slots_in, "use_interpolate"); const float thickness = BMO_slot_float_get(op->slots_in, "thickness"); const float depth = BMO_slot_float_get(op->slots_in, "depth"); ``` ---- before ``` /* use the largest angle */ mul_v3_fl(tvec, shell_v3v3_normalized_to_dist(tvec, len_squared_v3v3(tvec, e_info_a->no) > len_squared_v3v3(tvec, e_info_b->no) ? e_info_a->no : e_info_b->no)); ``` after ``` mul_v3_fl( tvec, shell_v3v3_normalized_to_dist( tvec, len_squared_v3v3( tvec, e_info_a->no) > len_squared_v3v3( tvec, e_info_b->no) ? e_info_a->no : e_info_b->no)); ``` ---- before ``` bm_loop_customdata_merge( bm, e_connect, l_a, BM_edge_other_loop(e_connect, l_a), l_a->prev, BM_edge_other_loop(e_connect, l_a->prev)); ``` after ``` bm_loop_customdata_merge( bm, e_connect, l_a, BM_edge_other_loop(e_connect, l_a), l_a->prev, BM_edge_other_loop(e_connect, l_a->prev)); ``` ---- `interface_handlers.c` before ``` else if ((!ELEM(event->type, MOUSEMOVE, WHEELUPMOUSE, WHEELDOWNMOUSE, MOUSEPAN)) && ISMOUSE(event->type)) { if (!ui_but_contains_point_px(but->active->region, but, event->x, event->y)) { but = NULL; } } ``` after ``` else if ((!ELEM(event->type, MOUSEMOVE, WHEELUPMOUSE, WHEELDOWNMOUSE, MOUSEPAN)) && ISMOUSE(event->type)) { if (!ui_but_contains_point_px( but->active->region, but, event->x, event->y)) { but = NULL; } } ``` before: ``` if (ISKEYBOARD(kmi->type)) { #if 0 /* would rather use a block but, but gets weirdly positioned... */ uiDefBlockBut(block, menu_change_shortcut, but, "Change Shortcut", 0, 0, uiLayoutGetWidth(layout), UI_UNIT_Y, ""); #endif but2 = uiDefIconTextBut(block, UI_BTYPE_BUT, 0, ICON_HAND, CTX_IFACE_(BLT_I18NCONTEXT_OPERATOR_DEFAULT, "Change Shortcut"), 0, 0, w, UI_UNIT_Y, NULL, 0, 0, 0, 0, ""); UI_but_func_set(but2, popup_change_shortcut_func, but, NULL); but2 = uiDefIconTextBut(block, UI_BTYPE_BUT, 0, ICON_NONE, CTX_IFACE_(BLT_I18NCONTEXT_OPERATOR_DEFAULT, "Remove Shortcut"), 0, 0, w, UI_UNIT_Y, NULL, 0, 0, 0, 0, ""); UI_but_func_set(but2, remove_shortcut_func, but, NULL); } else { but2 = uiDefIconTextBut(block, UI_BTYPE_BUT, 0, ICON_HAND, IFACE_("Non-Keyboard Shortcut"), 0, 0, w, UI_UNIT_Y, NULL, 0, 0, 0, 0, TIP_("Only keyboard shortcuts can be edited that way, " "please use User Preferences otherwise")); UI_but_flag_enable(but2, UI_BUT_DISABLED); } } /* only show 'add' if there's a suitable key map for it to go in */ else if (WM_keymap_guess_opname(C, but->optype->idname)) { but2 = uiDefIconTextBut(block, UI_BTYPE_BUT, 0, ICON_HAND, CTX_IFACE_(BLT_I18NCONTEXT_OPERATOR_DEFAULT, "Add Shortcut"), 0, 0, w, UI_UNIT_Y, NULL, 0, 0, 0, 0, ""); UI_but_func_set(but2, popup_add_shortcut_func, but, NULL); } ``` after ``` but2 = uiDefIconTextBut( block, UI_BTYPE_BUT, 0, ICON_HAND, CTX_IFACE_(BLT_I18NCONTEXT_OPERATOR_DEFAULT, "Change Shortcut"), 0, 0, w, UI_UNIT_Y, NULL, 0, 0, 0, 0, ""); UI_but_func_set(but2, popup_change_shortcut_func, but, NULL); but2 = uiDefIconTextBut( block, UI_BTYPE_BUT, 0, ICON_NONE, CTX_IFACE_(BLT_I18NCONTEXT_OPERATOR_DEFAULT, "Remove Shortcut"), 0, 0, w, UI_UNIT_Y, NULL, 0, 0, 0, 0, ""); UI_but_func_set(but2, remove_shortcut_func, but, NULL); } else { but2 = uiDefIconTextBut( block, UI_BTYPE_BUT, 0, ICON_HAND, IFACE_("Non-Keyboard Shortcut"), 0, 0, w, UI_UNIT_Y, NULL, 0, 0, 0, 0, TIP_("Only keyboard shortcuts can be edited that way, " "please use User Preferences otherwise")); UI_but_flag_enable(but2, UI_BUT_DISABLED); } } /* only show 'add' if there's a suitable key map for it to go in */ else if (WM_keymap_guess_opname(C, but->optype->idname)) { but2 = uiDefIconTextBut(block, UI_BTYPE_BUT, 0, ICON_HAND, CTX_IFACE_(BLT_I18NCONTEXT_OPERATOR_DEFAULT, "Add Shortcut"), 0, 0, w, UI_UNIT_Y, NULL, 0, 0, 0, 0, ""); UI_but_func_set(but2, popup_add_shortcut_func, but, NULL); } ```
Keir commented 5 years ago
Poster
Collaborator

Response to Campbell's questions:

  • Q: First question, do developers want this, will it save us time and make our life easier? :) - For patches/branches/contributions - yes. For existing code - think overall result may be slightly worse in many cases - though this is personal preference.
  • A: From my experience at Google, absolutely yes. It's a great time saver overall and leads to better, more consistent code.
  • Q: Could we use clang-format on a per-module basis? Instead of applying in source/ (maybe move towards applying globally, but at least not to begin with).
  • A: Yes; it's as simple as putting a .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.
  • Q: How much should we accept changing our style to fit in with what clang-format supports?
  • A: My opinion is that the benefit to having a tool that you can outsource manual formatting to is a huge win, and that the benefit of having a custom un-supported style in Blender is not worth it. Blender is entrenched, but if I were in charge I'd just set -style=google and re-format everything, but that's why I'm not in charge!
  • Q: There are times we would want to disable this, I suppose module owners can choose when? (if this is on average more than once per file... it would be cause for concern - but imagine its much less than this).
  • A: Yes; there are two ways. You can use /* 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).
  • Q: Some blocks of code are written for nice alignment and will read slightly worse with auto-formatting but probably not worth disabling auto-formatting. My main concern is we have slightly less readable code - because its too much hassle to add in comments. (examples below).
  • A: Suggestion here is to use manual control. This is what we do at Google. Though overall, it's discouraged; it's usually better to make your code work well with clang-format.
  • Q: 80 line length is quite a big change, would rather skip this for now. Its important but we can discuss separately.
  • A: Yes; the issue I see here is that clang-format will make everything 120, even though the current style guide says to try to keep things 80. My personal suggestion is to go for 80, but explicitly make it OK for maintainers to disable clang-format to go higher.
  • Q: Aligning arguments with the end of the functions causes a lot of right-shift for long function names. eg:
  • A: Unfortunately this is all or nothing. Personally, I find the right-pushed arguments are no problem. And note that clang-format will do the right thing if the argument pack is too big; it will push the entire pack onto the next line with an 8-space indent as you suggest.
Response to Campbell's questions: - Q: First question, do developers want this, will it save us time and make our life easier? :) - For patches/branches/contributions - yes. For existing code - think overall result may be slightly worse in many cases - though this is personal preference. - A: From my experience at Google, absolutely yes. It's a great time saver overall and leads to better, more consistent code. - Q: Could we use clang-format on a per-module basis? Instead of applying in source/ (maybe move towards applying globally, but at least not to begin with). - A: Yes; it's as simple as putting a `.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. - Q: How much should we accept changing our style to fit in with what clang-format supports? - A: My opinion is that the benefit to having a tool that you can outsource manual formatting to is a huge win, and that the benefit of having a custom un-supported style in Blender is not worth it. Blender is entrenched, but if I were in charge I'd just set `-style=google` and re-format everything, but that's why I'm not in charge! - Q: There are times we would want to disable this, I suppose module owners can choose when? (if this is on average more than once per file... it would be cause for concern - but imagine its much less than this). - A: Yes; there are two ways. You can use `/* 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). - Q: Some blocks of code are written for nice alignment and will read slightly worse with auto-formatting but probably not worth disabling auto-formatting. My main concern is we have slightly less readable code - because its too much hassle to add in comments. (examples below). - A: Suggestion here is to use manual control. This is what we do at Google. Though overall, it's discouraged; it's usually better to make your code work well with clang-format. - Q: 80 line length is quite a big change, would rather skip this for now. Its important but we can discuss separately. - A: Yes; the issue I see here is that clang-format will make everything 120, even though the current style guide says to try to keep things 80. My personal suggestion is to go for 80, but explicitly make it OK for maintainers to disable clang-format to go higher. - Q: Aligning arguments with the end of the functions causes a lot of right-shift for long function names. eg: - A: Unfortunately this is all or nothing. Personally, I find the right-pushed arguments are no problem. And note that clang-format will do the right thing if the argument pack is too big; it will push the entire pack onto the next line with an 8-space indent as you suggest.
Owner

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:

    if ( /* Constrain boxes to positive X/Y values */
        box_xmin_get(box) < 0.0f || box_ymin_get(box) < 0.0f ||
        /* check for last intersected */
        (vert->isect_cache[j] &&
         box_isect(box, vert->isect_cache[j])))
    {
        /* Here we check that the last intersected
         * box will intersect with this one using
         * isect_cache that can store a pointer to a
         * box for each quadrant
         * big speedup */
        isect = true;
    }

after:

    if (/* Constrain boxes to positive X/Y values */
        box_xmin_get(box) < 0.0f || box_ymin_get(box) < 0.0f ||
        /* check for last intersected */
        (vert->isect_cache[j] &&
         box_isect(box, vert->isect_cache[j]))) {
        /* Here we check that the last intersected
         * box will intersect with this one using
         * isect_cache that can store a pointer to a
         * box for each quadrant
         * big speedup */
        isect = true;
    }

before:

    if ((bezt->h1 != HD_VECT && bezt->h2 != HD_VECT) &&
        (dist_squared_to_line_v2(bezt->vec[0], bezt->vec[1], bezt->vec[2]) < (0.001f * 0.001f)) &&
        (len_squared_v2v2(bezt->vec[0], bezt->vec[1]) > eps_sq) &&
        (len_squared_v2v2(bezt->vec[1], bezt->vec[2]) > eps_sq) &&
        (len_squared_v2v2(bezt->vec[0], bezt->vec[2]) > eps_sq) &&
        (len_squared_v2v2(bezt->vec[0], bezt->vec[2]) >
         max_ff(len_squared_v2v2(bezt->vec[0], bezt->vec[1]),
                len_squared_v2v2(bezt->vec[1], bezt->vec[2]))))
    {
        bezt->h1 = bezt->h2 = HD_ALIGN;
    }

after:

    if ((bezt->h1 != HD_VECT && bezt->h2 != HD_VECT) &&
        (dist_squared_to_line_v2(
                 bezt->vec[0], bezt->vec[1], bezt->vec[2]) <
         (0.001f * 0.001f)) &&
        (len_squared_v2v2(bezt->vec[0], bezt->vec[1]) >
         eps_sq) &&
        (len_squared_v2v2(bezt->vec[1], bezt->vec[2]) >
         eps_sq) &&
        (len_squared_v2v2(bezt->vec[0], bezt->vec[2]) >
         eps_sq) &&
        (len_squared_v2v2(bezt->vec[0], bezt->vec[2]) >
         max_ff(len_squared_v2v2(bezt->vec[0], bezt->vec[1]),
                len_squared_v2v2(bezt->vec[1],
                                 bezt->vec[2])))) {
        bezt->h1 = bezt->h2 = HD_ALIGN;
    }


before

    if (len_squared_v3v3(ix, fv_b[i_b]->co) <= s->epsilon.eps2x_sq) {
        STACK_PUSH_TEST_A(fv_b[i_b]);
        STACK_PUSH_TEST_B(fv_b[i_b]);
    }

after

    if (len_squared_v3v3(ix, fv_b[i_b]->co) <=
        s->epsilon.eps2x_sq) {
        STACK_PUSH_TEST_A(fv_b[i_b]);
        STACK_PUSH_TEST_B(fv_b[i_b]);
    }

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:

    if (icon && name[0] && !icon_only)
        but = uiDefIconTextButR_prop(block,
                                     UI_BTYPE_ROW,
                                     0,
                                     icon,
                                     name,
                                     0,
                                     0,
                                     itemw,
                                     h,
                                     ptr,
                                     prop,
                                     -1,
                                     0,
                                     value,
                                     -1,
                                     -1,
                                     NULL);
    else if (icon)
        but = uiDefIconButR_prop(block,
                                 UI_BTYPE_ROW,
                                 0,
                                 icon,
                                 0,
                                 0,
                                 itemw,
                                 h,
                                 ptr,
                                 prop,
                                 -1,
                                 0,
                                 value,
                                 -1,
                                 -1,
                                 NULL);
    else
        but = uiDefButR_prop(block,
                             UI_BTYPE_ROW,
                             0,
                             name,
                             0,
                             0,
                             itemw,
                             h,
                             ptr,
                             prop,
                             -1,
                             0,
                             value,
                             -1,
                             -1,
                             NULL);

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:

- ifdef __GNUC__
- if (__GNUC__ * 100 + __GNUC_MINOR__) >= 406  /* gcc4.6+ only */
- pragma GCC diagnostic error "-Wsign-compare"
- endif
- if __GNUC__ >= 6  /* gcc6+ only */
- pragma GCC diagnostic error "-Wconversion"
- endif
- if (__GNUC__ * 100 + __GNUC_MINOR__) >= 408
     /* gcc4.8+ only (behavior changed to ignore globals)*/
#    pragma GCC diagnostic error "-Wshadow"
     /* older gcc changed behavior with ternary */
- pragma GCC diagnostic error "-Wsign-conversion"
- endif
/* pedantic gives too many issues, developers can define this for own use */
- ifdef WARN_PEDANTIC
- pragma GCC diagnostic error "-Wpedantic"
- ifdef __clang__  /* pedantic causes clang error */
- pragma GCC diagnostic ignored "-Wlanguage-extension-token"
- endif
- endif
#endif

after

- ifdef __GNUC__
- if (__GNUC__ * 100 + __GNUC_MINOR__) >= 406 /* gcc4.6+ only */
- pragma GCC diagnostic error "-Wsign-compare"
- endif
- if __GNUC__ >= 6 /* gcc6+ only */
- pragma GCC diagnostic error "-Wconversion"
- endif
- if (__GNUC__ * 100 + __GNUC_MINOR__) >= 408
/* gcc4.8+ only (behavior changed to ignore globals)*/
#pragma GCC diagnostic error "-Wshadow"
/* older gcc changed behavior with ternary */
- pragma GCC diagnostic error "-Wsign-conversion"
- endif
/* pedantic gives too many issues, developers can define this for own use */
- ifdef WARN_PEDANTIC
- pragma GCC diagnostic error "-Wpedantic"
- ifdef __clang__ /* pedantic causes clang error */
- pragma GCC diagnostic ignored "-Wlanguage-extension-token"
- endif
- endif
#endif

Indentation of Arrays

before:

static PyGetSetDef pyrna_struct_getseters[] = {
    {(char *)"id_data", (getter)pyrna_struct_get_id_data, (setter)NULL, (char *)pyrna_struct_get_id_data_doc, NULL},
    {NULL, NULL, NULL, NULL, NULL} /* Sentinel */
};

after: (for some reason it's using 8 spaces indentation... maybe simple to solve)

static PyGetSetDef pyrna_struct_getseters[] = {
        {(char *)"id_data",
         (getter)pyrna_struct_get_id_data,
         (setter)NULL,
         (char *)pyrna_struct_get_id_data_doc,
         NULL},
        {NULL, NULL, NULL, NULL, NULL} /* Sentinel */
};

Non-Blockers

Some practical concerns which we can work-around:

Bugs in Clang-Format

  • Errors in indentation especially in code that uses pre-processor defines. These files might have to disable formatting.
  • 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.

  • It is yet another thing for new contributors to have to setup (currently Linux users don't need to download binary libs for example).
  • In the future, new versions of clang-format will be released, some developers will have access easily from their package manager and not want to go through the hassle of downloading SVN binary directories for software they can install with a single command which is almost (but not exactly) the same as a slightly older version.
  • Distributing binaries for clang format means:
    • We build for all supported platforms (at least 3, maybe we don't bother with 32bit)
    • When there is a significant update to clang-format, its more work for platform maintainers
    • If clang-format fails for any reason, its work for us to support users and figure out what library/libc... etc is missing.

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.

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: ``` if ( /* Constrain boxes to positive X/Y values */ box_xmin_get(box) < 0.0f || box_ymin_get(box) < 0.0f || /* check for last intersected */ (vert->isect_cache[j] && box_isect(box, vert->isect_cache[j]))) { /* Here we check that the last intersected * box will intersect with this one using * isect_cache that can store a pointer to a * box for each quadrant * big speedup */ isect = true; } ``` after: ``` if (/* Constrain boxes to positive X/Y values */ box_xmin_get(box) < 0.0f || box_ymin_get(box) < 0.0f || /* check for last intersected */ (vert->isect_cache[j] && box_isect(box, vert->isect_cache[j]))) { /* Here we check that the last intersected * box will intersect with this one using * isect_cache that can store a pointer to a * box for each quadrant * big speedup */ isect = true; } ``` ---- before: ``` if ((bezt->h1 != HD_VECT && bezt->h2 != HD_VECT) && (dist_squared_to_line_v2(bezt->vec[0], bezt->vec[1], bezt->vec[2]) < (0.001f * 0.001f)) && (len_squared_v2v2(bezt->vec[0], bezt->vec[1]) > eps_sq) && (len_squared_v2v2(bezt->vec[1], bezt->vec[2]) > eps_sq) && (len_squared_v2v2(bezt->vec[0], bezt->vec[2]) > eps_sq) && (len_squared_v2v2(bezt->vec[0], bezt->vec[2]) > max_ff(len_squared_v2v2(bezt->vec[0], bezt->vec[1]), len_squared_v2v2(bezt->vec[1], bezt->vec[2])))) { bezt->h1 = bezt->h2 = HD_ALIGN; } ``` after: ``` if ((bezt->h1 != HD_VECT && bezt->h2 != HD_VECT) && (dist_squared_to_line_v2( bezt->vec[0], bezt->vec[1], bezt->vec[2]) < (0.001f * 0.001f)) && (len_squared_v2v2(bezt->vec[0], bezt->vec[1]) > eps_sq) && (len_squared_v2v2(bezt->vec[1], bezt->vec[2]) > eps_sq) && (len_squared_v2v2(bezt->vec[0], bezt->vec[2]) > eps_sq) && (len_squared_v2v2(bezt->vec[0], bezt->vec[2]) > max_ff(len_squared_v2v2(bezt->vec[0], bezt->vec[1]), len_squared_v2v2(bezt->vec[1], bezt->vec[2])))) { bezt->h1 = bezt->h2 = HD_ALIGN; } ``` ---- before ``` if (len_squared_v3v3(ix, fv_b[i_b]->co) <= s->epsilon.eps2x_sq) { STACK_PUSH_TEST_A(fv_b[i_b]); STACK_PUSH_TEST_B(fv_b[i_b]); } ``` after ``` if (len_squared_v3v3(ix, fv_b[i_b]->co) <= s->epsilon.eps2x_sq) { STACK_PUSH_TEST_A(fv_b[i_b]); STACK_PUSH_TEST_B(fv_b[i_b]); } ``` ---- ### 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: ``` if (icon && name[0] && !icon_only) but = uiDefIconTextButR_prop(block, UI_BTYPE_ROW, 0, icon, name, 0, 0, itemw, h, ptr, prop, -1, 0, value, -1, -1, NULL); else if (icon) but = uiDefIconButR_prop(block, UI_BTYPE_ROW, 0, icon, 0, 0, itemw, h, ptr, prop, -1, 0, value, -1, -1, NULL); else but = uiDefButR_prop(block, UI_BTYPE_ROW, 0, name, 0, 0, itemw, h, ptr, prop, -1, 0, value, -1, -1, NULL); ``` 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: ``` - ifdef __GNUC__ - if (__GNUC__ * 100 + __GNUC_MINOR__) >= 406 /* gcc4.6+ only */ - pragma GCC diagnostic error "-Wsign-compare" - endif - if __GNUC__ >= 6 /* gcc6+ only */ - pragma GCC diagnostic error "-Wconversion" - endif - if (__GNUC__ * 100 + __GNUC_MINOR__) >= 408 /* gcc4.8+ only (behavior changed to ignore globals)*/ # pragma GCC diagnostic error "-Wshadow" /* older gcc changed behavior with ternary */ - pragma GCC diagnostic error "-Wsign-conversion" - endif /* pedantic gives too many issues, developers can define this for own use */ - ifdef WARN_PEDANTIC - pragma GCC diagnostic error "-Wpedantic" - ifdef __clang__ /* pedantic causes clang error */ - pragma GCC diagnostic ignored "-Wlanguage-extension-token" - endif - endif #endif ``` after ``` - ifdef __GNUC__ - if (__GNUC__ * 100 + __GNUC_MINOR__) >= 406 /* gcc4.6+ only */ - pragma GCC diagnostic error "-Wsign-compare" - endif - if __GNUC__ >= 6 /* gcc6+ only */ - pragma GCC diagnostic error "-Wconversion" - endif - if (__GNUC__ * 100 + __GNUC_MINOR__) >= 408 /* gcc4.8+ only (behavior changed to ignore globals)*/ #pragma GCC diagnostic error "-Wshadow" /* older gcc changed behavior with ternary */ - pragma GCC diagnostic error "-Wsign-conversion" - endif /* pedantic gives too many issues, developers can define this for own use */ - ifdef WARN_PEDANTIC - pragma GCC diagnostic error "-Wpedantic" - ifdef __clang__ /* pedantic causes clang error */ - pragma GCC diagnostic ignored "-Wlanguage-extension-token" - endif - endif #endif ``` ## Indentation of Arrays before: ``` static PyGetSetDef pyrna_struct_getseters[] = { {(char *)"id_data", (getter)pyrna_struct_get_id_data, (setter)NULL, (char *)pyrna_struct_get_id_data_doc, NULL}, {NULL, NULL, NULL, NULL, NULL} /* Sentinel */ }; ``` after: (for some reason it's using 8 spaces indentation... maybe simple to solve) ``` static PyGetSetDef pyrna_struct_getseters[] = { {(char *)"id_data", (getter)pyrna_struct_get_id_data, (setter)NULL, (char *)pyrna_struct_get_id_data_doc, NULL}, {NULL, NULL, NULL, NULL, NULL} /* Sentinel */ }; ``` ---- # Non-Blockers Some practical concerns which we can work-around: ### Bugs in Clang-Format - Errors in indentation especially in code that uses pre-processor defines. These files might have to disable formatting. - `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](https://archive.blender.org/developer/P551.txt) (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. - It is yet another thing for new contributors to have to setup (currently Linux users don't need to download binary libs for example). - In the future, new versions of clang-format will be released, some developers will have access easily from their package manager and not want to go through the hassle of downloading SVN binary directories for software they can install with a single command which is almost (but not exactly) the same as a slightly older version. - Distributing binaries for clang format means: - We build for all supported platforms (at least 3, maybe we don't bother with 32bit) - When there is a significant update to clang-format, its more work for platform maintainers - If clang-format fails for any reason, its work for us to support users and figure out what library/libc... etc is missing. ---- # 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.
Collaborator

Added subscriber: @LazyDodo

Added subscriber: @LazyDodo
Collaborator

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

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
Owner

@LazyDodo not sure what you mean about "solving all our problems" ? - I've been testing the latest release of clang-format v5.0.

@LazyDodo not sure what you mean about *"solving all our problems"* ? - I've been testing the latest release of clang-format v5.0.
Collaborator

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.

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.
brecht commented 5 years ago
Owner

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.

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

Added subscriber: @Stefan_Werner

Added subscriber: @Stefan_Werner
Owner

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,
                                     }};
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](https://archive.blender.org/developer/P560.txt) 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, }}; ```
mont29 commented 5 years ago
Owner

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.

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): ```lang=c 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… ```lang=c 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); } ``` ----------------------- ```lang=c 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… ```lang=c 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; } ``` ----------------------- ```lang=c if (local->override->reference->override && (local->override->reference->tag & LIB_TAG_OVERRIDE_OK) == 0) { BKE_override_update(bmain, local->override->reference); } ``` giving… ```lang=c 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.
Owner

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

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

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

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

Reported bug to clang-format regarding C99 struct alignment: https://bugs.llvm.org/show_bug.cgi?id=37134

Reported bug to clang-format regarding C99 struct alignment: https://bugs.llvm.org/show_bug.cgi?id=37134
Collaborator

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

Looks like we can work aorund the C99 struct alignment issue by setting [`BreakBeforeBinaryOperators: All` ](https://stackoverflow.com/questions/48587210/clang-format-rule-for-nested-struct-fields)
Owner

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

struct ApplicationState app_state = {
        .signal =
                {
                        .use_crash_handler = true,
                        .use_abort_handler = true,
                },
        .exit_code_on_error =
                {
                        .python = 0,
                },
};
@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`). ``` struct ApplicationState app_state = { .signal = { .use_crash_handler = true, .use_abort_handler = true, }, .exit_code_on_error = { .python = 0, }, }; ```
Owner

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/

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/
Sergey commented 4 years ago
Owner

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.

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

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.

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

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?

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?
Collaborator

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

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

In #53211#531885, @LazyDodo wrote:
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


In #53211#534069, @dfelinto wrote:
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.

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.

> In #53211#531885, @LazyDodo wrote: > Looks like we can work aorund the C99 struct alignment issue by setting [`BreakBeforeBinaryOperators: All` ](https://stackoverflow.com/questions/48587210/clang-format-rule-for-nested-struct-fields) 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 ---- > In #53211#534069, @dfelinto wrote: > 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. 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`.
Sergey commented 4 years ago
Owner

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.
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.
Keir commented 4 years ago
Poster
Collaborator

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

@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.
mont29 commented 4 years ago
Owner

In #53211#594542, @ideasman42 wrote:

In #53211#534069, @dfelinto wrote:
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.

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 @Sergey I totally agree automatic formatting is a great tool to have - as long as it does produce good readable code. ;)

> In #53211#594542, @ideasman42 wrote: >> In #53211#534069, @dfelinto wrote: >> 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. > > 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 @Sergey I totally agree automatic formatting is a great tool to have - as long as it does produce good readable code. ;)
mont29 commented 4 years ago
Owner

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

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

In #53211#594732, @mont29 wrote:

In #53211#594542, @ideasman42 wrote:

In #53211#534069, @dfelinto wrote:
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.

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 @Sergey I totally agree automatic formatting is a great tool to have - as long as it does produce good readable code. ;)

+1

> In #53211#594732, @mont29 wrote: >> In #53211#594542, @ideasman42 wrote: >>> In #53211#534069, @dfelinto wrote: >>> 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. >> >> 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 @Sergey I totally agree automatic formatting is a great tool to have - as long as it does produce good readable code. ;) +1
Owner

In #53211#594690, @Keir wrote:
@ideasman42, 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.

> In #53211#594690, @Keir wrote: > @ideasman42, 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.
Owner

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

  • Conditional braces always on newline as @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 @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.
Tried tweaking options and here are two which I think are acceptable. - Conditional braces always on newline as @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 ](https://bugs.llvm.org/show_bug.cgi?id=37134) (as @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](https://archive.blender.org/developer/P884.txt) (4 char indent, 120 width). - [P885](https://archive.blender.org/developer/P885.txt) (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.
brecht commented 4 years ago
Owner

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

In #53211#594777, @brecht wrote:
+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 e305560f13

> In #53211#594777, @brecht wrote: > +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](https://archive.blender.org/developer/P884.txt) config, although many more foreach macros are needed. Also committed trailing commas to workaround strange struct indentation e305560f13
mont29 commented 4 years ago
Owner

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.

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

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.

Updated [P884](https://archive.blender.org/developer/P884.txt) - 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 commented 4 years ago
Poster
Collaborator

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 updated [P884](https://archive.blender.org/developer/P884.txt) and [P885](https://archive.blender.org/developer/P885.txt) to turn on `IndentPPDirectives`, which is now commonly available in deployed Clang versions (available as of Clang 6). This improves preprocessor formatting.
Sergey commented 4 years ago
Owner

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 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).
brecht commented 4 years ago
Owner

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

In #53211#594895, @brecht wrote:

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

> In #53211#594895, @brecht wrote: >> 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 ](https://stackoverflow.com/a/28437960) , not ideal, but we'd have to run it once during migration to the new rules so it may not be that bad...
Owner

In #53211#594895, @brecht wrote:

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

> In #53211#594895, @brecht wrote: >> 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](https://archive.blender.org/developer/P884.txt)).
Owner

edit, IndentPPDirectives now works

*edit, IndentPPDirectives now works*
Owner

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).
In general [P884](https://archive.blender.org/developer/P884.txt) 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).
Sergey commented 4 years ago
Owner

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

@ideasman42, 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.

@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. @ideasman42, 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.
Owner

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.

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](https://archive.blender.org/developer/P884.txt)) - `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.
Owner

In #53211#595054, @ideasman42 wrote:
Otherwise, next steps.

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

> In #53211#595054, @ideasman42 wrote: > Otherwise, next steps. Let's not forget clear instructions for new and old developers: updated code-style, IDE, and build wiki documents.
Keir commented 4 years ago
Poster
Collaborator

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:

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;
}
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: ``` 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; } ```
Owner

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

@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)*.
Collaborator

Added subscriber: @dr.sybren

Added subscriber: @dr.sybren
Collaborator

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.

> 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.
Keir commented 4 years ago
Poster
Collaborator

@LazyDodo Good catch on the .editconfig discrepancy. I've extended #60279 to include updating other IDE configs that exist in the Blender source.

@LazyDodo Good catch on the `.editconfig` discrepancy. I've extended #60279 to include updating other IDE configs that exist in the Blender source.
Collaborator

Added subscriber: @JulianEisel

Added subscriber: @JulianEisel
Sergey commented 4 years ago
Owner

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?

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?
brecht commented 4 years ago
Owner

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

@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`.
Owner
  • 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.
- 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.
Collaborator

Added subscriber: @JacquesLucke

Added subscriber: @JacquesLucke
Owner

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:

Added [D4185](https://archive.blender.org/developer/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 - edit: reported https://bugs.llvm.org/show_bug.cgi?id=40288 - edit: worked around 302970b7a5
brecht commented 4 years ago
Owner

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.

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.
Sergey commented 4 years ago
Owner

@brecht, opensubdiv's C-API is also following google's style.

@brecht, opensubdiv's C-API is also following google's style.
brecht commented 4 years ago
Owner

Ok, I've excluded opensubdiv as well now.

Ok, I've excluded opensubdiv as well now.
Sergey commented 4 years ago
Owner

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?

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? - [x] https://github.com/llvm-mirror/clang/blob/master/tools/clang-format/git-clang-format
Owner

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.

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

In #53211#597808, @Sergey wrote:
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, 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.

> In #53211#597808, @Sergey wrote: > 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? > > - [x] 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`.
Sergey commented 4 years ago
Owner

Another thing i'd love to propose is to enable FixNamespaceComments.

Another thing i'd love to propose is to enable `FixNamespaceComments`.
Owner

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.

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

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'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](https://archive.blender.org/developer/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.
Collaborator

Added subscriber: @Jeroen-Bakker

Added subscriber: @Jeroen-Bakker
Collaborator

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.

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.
mont29 commented 4 years ago
Owner

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?

All in all, am okay with it… still not enchanted (by far ;) ), but it's okay. We can always work around stupidities like that: ```lang=C ``` 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?
Owner

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

@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.
mont29 commented 4 years ago
Owner

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

  • Merge temp-clang-format into asset-engine (about 6 conflicting files, essentially due to recent pre-clang-format edits in master).
  • Apply clang-format on temp-clang-format and commit.
  • Apply clang-format on asset-engine and commit.
  • 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!

@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: - Merge `temp-clang-format` into `asset-engine` (about 6 conflicting files, essentially due to recent pre-clang-format edits in master). - Apply clang-format on `temp-clang-format` and commit. - Apply clang-format on `asset-engine` and commit. - 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!
Owner

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.

561e02831c

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. 561e02831c
Sergey commented 4 years ago
Owner

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

  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.

@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: ``` 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.
Owner

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.

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.

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. ``` 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.
Sergey commented 4 years ago
Owner

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.

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

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.

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.
Sergey commented 4 years ago
Owner

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.

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

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


In #53211#612559, @Sergey wrote:

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

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`). ---- > In #53211#612559, @Sergey wrote: > >> 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 :)
Sergey commented 4 years ago
Owner

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!

> 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!
Sergey commented 4 years ago
Owner

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?

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?
Owner

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.

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.
Sergey commented 4 years ago
Owner

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:

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 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'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: ``` 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 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).
brecht commented 4 years ago
Owner

I do indeed prefer Align personally.

I've committed a change that makes make format run multithreaded, now it completes in ~10s for me.

I do indeed prefer `Align` personally. I've committed a change that makes `make format` run multithreaded, now it completes in ~10s for me.
Collaborator

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.

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

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:
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: ```
brecht commented 4 years ago
Owner

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

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

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) SSD Spinner
--- -------
Single Threaded 154.25 237.17
MultiT hreaded 43.18 125.06
MultiThreaded Batched 28.2 126.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)

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)|SSD|Spinner | -- | -- | -- | ||---|------- |Single Threaded|154.25|237.17 |MultiT hreaded|43.18|125.06 |MultiThreaded Batched|28.2|126.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)
brecht commented 4 years ago
Owner

TabExpand will be removed once the clang migration is done, so not worth optimizing.

TabExpand will be removed once the clang migration is done, so not worth optimizing.
Collaborator

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?

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?
Owner

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

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

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
mont29 commented 4 years ago
Owner

Regarding Align versus AlwaysBreak, I don’t have any strong opinion here, imho we'll have some 'bad' results in both cases, so… Am feeling neutral here. ;)

Regarding `Align` versus `AlwaysBreak`, I don’t have any strong opinion here, imho we'll have some 'bad' results in both cases, so… Am feeling neutral here. ;)
Owner

Changed status from 'Open' to: 'Resolved'

Changed status from 'Open' to: 'Resolved'
ideasman42 closed this issue 4 years ago
ideasman42 self-assigned this 4 years ago
Owner

ClangFormat is now in use: e12c08e8d1

ClangFormat is now in use: e12c08e8d1

Added subscriber: @MyDeveloperDay

Added subscriber: @MyDeveloperDay
@ideasman42 is this of interest? https://reviews.llvm.org/D68296
Owner

@MyDeveloperDay great! although there would be some delay before we would apply this change.

@MyDeveloperDay great! although there would be some delay before we would apply this change.

Just reviewing your other concerns with clang-format

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.

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

# This tries to match Blender's style as much as possible. One
BreakBeforeBraces: Custom
BraceWrapping: {
    AfterClass: 'false'
    AfterCaseLabel: 'true'
    AfterControlStatement: 'false'
    AfterEnum : 'false'
    AfterFunction : 'true'

With this setting the example from your style guide formats as follows

switch (value) {
  case TEST_A:
  {
    int a = func();
    result = a + 10;
    break;
  }
  case TEST_B:
    func_b();
    ATTR_FALLTHROUGH;
  case TEST_C:
  case TEST_D:
  {
    func_c();
    break;
  }
}
Just reviewing your other concerns with clang-format > 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. 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.0 ``` # This tries to match Blender's style as much as possible. One BreakBeforeBraces: Custom BraceWrapping: { AfterClass: 'false' AfterCaseLabel: 'true' AfterControlStatement: 'false' AfterEnum : 'false' AfterFunction : 'true' ``` With this setting the example from your style guide formats as follows ``` switch (value) { case TEST_A: { int a = func(); result = a + 10; break; } case TEST_B: func_b(); ATTR_FALLTHROUGH; case TEST_C: case TEST_D: { func_c(); break; } } ```
Sign in to join this conversation.
No Label
good first issue
legacy module/Animation & Rigging
legacy module/Core
legacy module/Development Management
legacy module/Eevee & Viewport
legacy module/Grease Pencil
legacy module/Modeling
legacy module/Nodes & Physics
legacy module/Pipeline, Assets & IO
legacy module/Platforms, Builds, Tests & Devices
legacy module/Python API
legacy module/Rendering & Cycles
legacy module/Sculpt, Paint & Texture
legacy module/Triaging
legacy module/User Interface
legacy module/VFX & Video
legacy project/1.0.0-beta.2
legacy project/2.81
legacy project/2.82
legacy project/2.83
legacy project/2.90
legacy project/2.91
legacy project/2.92
legacy project/3.0
legacy project/3.1
legacy project/3.2
legacy project/3.3
legacy project/Alembic
legacy project/Animation & Rigging
legacy project/Asset Browser
legacy project/Asset Browser (Archived)
legacy project/Asset Browser Project Overview
legacy project/Audio
legacy project/Automated Testing
legacy project/BF Blender: 2.8
legacy project/BF Blender: After Release
legacy project/BF Blender: Next
legacy project/BF Blender: Regressions
legacy project/BF Blender: Unconfirmed
legacy project/Blender 2.70
legacy project/Blender Asset Bundle
legacy project/Code Quest
legacy project/Collada
legacy project/Compositing
legacy project/Core
legacy project/Cycles
legacy project/Datablocks and Libraries
legacy project/Dependency Graph
legacy project/Development Management
legacy project/Eevee
legacy project/EEVEE & Viewport
legacy project/Freestyle
legacy project/Game Animation
legacy project/Game Audio
legacy project/Game Data Conversion
legacy project/Game Engine
legacy project/Game Logic
legacy project/Game Physics
legacy project/Game Python
legacy project/Game Rendering
legacy project/Game UI
legacy project/Geometry Nodes
legacy project/Good First Issue
legacy project/GPU / Viewport
legacy project/Grease Pencil
legacy project/GSoC
legacy project/Images & Movies
legacy project/Import/Export
legacy project/Infrastructure: Websites
legacy project/LibOverrides - Usability and UX
legacy project/Line Art
legacy project/Masking
legacy project/Milestone 1: Basic, Local Asset Browser
legacy project/Modeling
legacy project/Modifiers
legacy project/Motion Tracking
legacy project/Nodes
legacy project/Nodes & Physics
legacy project/OpenGL Error
legacy project/Overrides
legacy project/Papercut
legacy project/Performance
legacy project/Physics
legacy project/Pipeline, Assets & I/O
legacy project/Platform: FreeBSD
legacy project/Platform: Linux
legacy project/Platform: macOS
legacy project/Platforms, Builds, Tests & Devices
legacy project/Platform: Windows
legacy project/Pose Library Basics
legacy project/Python API
legacy project/Render & Cycles
legacy project/Render Pipeline
legacy project/Retrospective
legacy project/Sculpt, Paint & Texture
legacy project/Text Editor
legacy project/Tracker Curfew
legacy project/Translations
legacy project/Triaging
legacy project/Undo
legacy project/USD
legacy project/User Interface
legacy project/UV Editing
legacy project/VFX & Video
legacy project/Video Sequencer
legacy project/Virtual Reality
legacy project/Wintab High Frequency
migration/requires-manual-verification
Module › Animation & Rigging
Module › Core
Module › Development Management
Module › Eevee & Viewport
Module › EEVEE & Viewport
Module › Grease Pencil
Module › Modeling
Module › Nodes & Physics
Module › Pipeline, Assets & IO
Module › Platforms, Builds Tests & Devices
Module › Platforms, Builds, Tests & Devices
Module › Python API
Module › Render & Cycles
Module › Sculpt, Paint & Texture
Module › Triaging
Module › User Interface
Module › VFX & Video
papercut
performance
Priority › High
Priority › Low
Priority › Normal
Priority › Unbreak Now!
Status › Archived
Status › Confirmed
Status › Duplicate
Status › Needs Information 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…
There is no content yet.