Page MenuHome

RFC: Clang-format for Blender
Open, NormalPublic

Description

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

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

Overview
Currently the code in Blender has a style guide, but there are only limited tools to enforce this style. In the past, there have been various tools for reformatting C and C++ code, but for the most part they did not work well. This was usually because they did not implement a full C/C++ parser, and instead tried to parse a subset of the language in order to provide the formatting. Unfortunately, this only works for simple code; the moment sophisticated code is used, these simplistic formatting solutions don't work well.

In the intervening years, a new entrant has stepped onto the stage: clang-format. clang-format is different than the previous versions for a key reason: it is built around a full-scale industrial strength C/C++ parsing engine, clang. And unlike other attempts, clang-format is used and extended by some of the largest companies for their large scale codebases: Apple, Google, Mozilla, and more.

Additionally, beyond just leveraging the full C/C++ parser of Clang, clang-format aims to support a limited number of formatting styles, but support those styles extremely well; handling as many edge cases correctly as possible, to avoid spending engineering time fixing autoformatting mistakes.

For all these reasons, this proposal is to figure out a plan to bring clang-format to Blender.

Why have consistent formatting?
The key reason is to have consistent code that is readable by all Blender developers, and to help save time by removing time spent debating style questions.

Why switch to automatic formatting?
The simple reason is that it's a waste of precious developer time to have them obsess over formatting details. In the past, automatic tools did not work well enough, but that is no longer true. Let's both save time and increase code quality by leveraging the immense engineering effort of the clang format team to increase Blender's code consistency.

Proposed rollout plan

  1. Commit a .clang-format file to Blender's repository (probably root Blender repo). Note that clang-format doesn't work well in an external repo; it does not support specifying a style file, and instead scans directories recursively upward from a file to find the closest parent directory with a .clang-format file.
  2. Module owners can chose to run clang-format on files they are working on if they want, but running mass format updates is strongly discouraged.
  3. Clang format results should not be blindly applied, and all changes by clang-format should be reviewed carefully.
  4. New contributors are encouraged to use clang-format on their changes, but should not run clang-format on other unrelated parts of their code. At first, this will not work as well since much of the existing Blender code doesn't adhere to its own style guide, but over time as more files are run through clang format, this will become less problematic.
  5. Make a wiki page to describe how to use clang-format, and explaining how NOT to use it (e.g. banning wholesale reformatting of many files, etc).
  6. Over time, more and more files would become "clang-format clean" where running clang-format on the file will produce no changes.
  7. Eventually, after the version of LLVM is updated to the latest, switch to using the clang-format that's included in the libs (probably going to clang-format 6.0 if possible).

I want to try it!
See the below sample config and instructions.
UPDATE JAN 7/2019: See the new temp-clang-format branch created by Campbell; it has the current source-of-truth version of the .clang-format.

Guidelines for using clang-format
End-goal: running clang-format on a file will result in no changes to that file. This will not always be possible due to some limitations in clang-format, but this is the desired result.

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

Do / Do not

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

Issues with clang-format
While clang-format is comprehensive, not everything can be set to match Blender's style, and additionally there are some minor bugs that can crop up.

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

Long term we'd like to fix these upstream, but upstream fixes will take a long time to propagate through to a stable release. So for now, unfortunately these issues will have to be manually addressed.

FAQ

  • Q: What about forks or branches?
  • A: There isn't an easy answer here. Solution is either for branches to merge first, then reformat, or to run clang-format independently on the patches.
  • Q: What about the Blender style requirements that don't work with clang-format yet?
  • A: The most problematic ones are (a) the indent-after # issue with the preprocessor and (b) the newline before { for if statement conditions that span multiple lines. (a) is already supported in upstream clang-format, but it is not available everywhere yet without compiling LLVM (a big job). (b) could is not implemented, but we could probably convince upstream to take a patch for that. UPDATE JAN 7/2019: This is no longer an issue since Clang >=6 is widely available now.
  • Q: What about enforcing non-whitespace changes like requiring {} everywhere even for one-line statements?
  • A: Non-whitespace changes aren't supported by clang-format. However, these type of issues can be corrected with clang-tidy, which is a bit more powerful. That's a something that can get looked at later; for now, this proposal is sticking to just clang-format, since it is more widely available and more stable.
  • Q: What about platform support (e.g. Mac, Windown, Linux)?
  • A: On Mac, clang-format is available by default as part of XCode. On Linux it is available with sudo apt-get install clang-format. On Windows, it is available as part of the Windows builds.

Editor integration

Config
The configuration options are documented at the LLVM site.

Proposed config for Blender that gets most of the clang-format settings pretty close is below. To use it, create a file .clang-format in the root Blender repo (next to make.bat). Then run clang-format -i <file>.

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

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

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

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

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

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

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

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

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

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

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

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

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

SpaceAfterTemplateKeyword: 'false'

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

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

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

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

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

Details

Related Objects

Event Timeline

Sergey Sharybin (sergey) triaged this task as Normal priority.Oct 31 2017, 11:16 AM

@Keir Mierle (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 Mierle (keir) updated the task description. (Show Details)

I can't find clang-format in my Xcode 9 install?

For macOS and Windows we can install it along with precompiled libraries. In fact it's already there on Windows, but that's from LLVM 3.5. We will need to upgrade to a newer LLVM/Clang version, which is needed for newer OSL anyway.

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

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);
	}

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.

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.

before we get all excited and go we can upgrade clang-format with never versions and solve all our problems, the only reason we currently have llvm/clang in our libs, is because it's a requirement for OSL. Which i assume will stay the leading dependency for clang, so unless OSL supports it (and they have been lagging with supporting new versions in the past by quite a bit) there will be no upgrades to clang-format either. something to probably keep in mind

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

there seems to be a general vibe of, 'there is some issues with the current version, but if we submit some patches a future version could handle all our cases that are currently broken' i'm just pointing out that some of our other deps might be holding back updating to the latest and greatest.

OSL was stuck on an old LLVM version for a long time, because LLVM completely rewrote the JIT compiler to share code with the regular compiler. There were performance issues with that, and they have been solved now. It should be ok to upgrade to LLVM 5.0, and I don't expect major issues upgrading to newer versions in the future.

Just talked with mont, dalai & sergey, we're thinking of trying out something closer to googles code style since it solves the accidental allignment of if-statements.

Differences are:

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

P560

also made other modifications:

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

A note on argument wrapping, passing 10+ arguments to a function isn't great.
In the case of interface code, there is x/y/w/h which could be written as a compound struct,

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

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


Remaining issues:

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

before:

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

after:

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

While not happy about it, I do can live with 2-spaces indentation and 80 chars width. But I still think this is less readable, 2-spaces make it much harder to tell at which level, i.e. which block, you are, and 80 chars is very small, especially when using longer, more verbose/descriptive names. What you gain in width, you lose in high, not a winner trade imho. Especially since it means you see much less real code at once on your screen, and have to scroll even more.

Granted, we’d have to adapt to new style, readability also is a matter of taking it into account when writing code. But… Here are a few examples which I really find much, much worse after clang-format (using @cambellbarton's one, and clang-format 3.8, which is default one on Debian testing):

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

giving…

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

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

giving…

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

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

giving…

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

And even with all those lines with only { on it removed, file still goes from 665 lines to 724…


So to summarize, am not convinced, and do not even understand how a style-fixing tool can miss a point like 'put opening brace on next line for multi-lines statements'… For me this is very 101 of readability, much, much more than putting one arg per line in function calls or declarations! Unless you simply forbid any complex if/for/etc. statement, and enforces to do any complex handling in separate steps. But this kind of goes beyond code style, and certainly cannot be handled by automatic tool anymore.

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

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

Brace placement is important for readability - we could hold off using clang-format until this is handled, check if it's something they would support.

Update, tested clang6 and compared output with clang5, it improved on indenting some comment blocks. Otherwise issues we ran into aren't resolved.

(some of them should be reported to clang, also it's possible there are new options in clang6).

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

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

@Campbell Barton (campbellbarton) if we have a trailing comma in all the initialized struct members the result is far better (in this case I added a comma after the } of .exit_code_on_error).

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

I talked to @Campbell Barton (campbellbarton) and he proposed we had a post-processing script to handle the cases where clang-format falls short. This would allow us to set a code style for Blender that will still be valid even after clang-format supports some of our requirements. Over time our goal would then be to reduce the requirement of the post-processing script as clang-format gets fixed.

My concern is only with this conflicting with the clang-format support for IDEs. I wonder what @Keir Mierle (keir) thinks of it.

PS.: Interesting read up on MongoDB clang-format adoption - https://engineering.mongodb.com/post/succeeding-with-clangformat-part-1-pitfalls-and-planning/

And then have script which handles failures of post-processing script...

The whole idea of clang-format is to make it simple to bring source to a consistent state, without any extra work from our side, utilizing support of IDEs as much as possible.

Are we really going to write own fully-featured C11 parser, with support of preprocessor and such? Using Python 3, requiring Windows users to have it installed in PATH? Or use clang lexer python bindings, which i do not see existing for python3.

I do not find the idea of a separate script correct at all.

msvc has build in clang-format support nowdays, but doesn't offer any post processing options, so i really would prefer not to go down the scripting route.

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

  • Brace Placement w/ Multi-Line Checks

If we switch to 2-space indentation this problem disappear.

  • Function Arg Wrapping

I don't think is an issue at all. The problem comes from our (ab)use of extremely short variables. In fact this even stimulates the developer to use more clear variable names.

  • Preprocessor

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

  • Array-indentation

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

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

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

This enforces all operators to be on a newline, which I'm not a fan of (often mis-aligns statements that would otherwise be aligned, goes against current convention) - giving us less readable code to workaround a clang bug.

See docs: http://releases.llvm.org/7.0.0/tools/clang/docs/ClangFormatStyleOptions.html


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

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

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

  • Function Arg Wrapping I don't think is an issue at all. The problem comes from our (ab)use of extremely short variables. In fact this even stimulates the developer to use more clear variable names.

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

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

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

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

Yes, it's just strange that array definitions use different indentation to other blocks. Not sure if there is a way to indent arrays the same as regular code blocks? Without having ContinuationIndentWidth set to IndentWidth.

Some inlined answers

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

Would absolutely love to!

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

I would consider this as a point against of heavy preprocessor blocks. There are only handful of those which are useful, the rest are just causing extra overhead for debugging.


It seems the arguments against this proposal are based on the behaviour of clang-format on a code which is already not very good. To me the way to deal with this would be:

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

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

Pros:

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

Cons:

  • Badly written code becomes more unreadable.

Personally, i value the time for actual development gained from automated technical task of ensuring code style more than keeping old ugly code in its bubble.


Just some examples of us(me) spending a lot of time on this topic:

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

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

While on one hand, I'm happy to see that this issue is not closed with "no way!" yet, I'm also sad that there still isn't a resolution after more than a year. There is a strong dollars-and-cents case for automatic formatting, which is why so many companies have adopted it. For a volunteer project such as Blender, where core developer time is incredibly valuable, and onboarding new developers is critical to the lifeblood of the project, I hoped it would be clear that adopting something like this proposal is worthwhile. While there are edge cases that may require some compromise or a sprinkling of /*clang-format: off */, it's important to not over-weight the small up-front cost of this versus the high and ongoing tax paid by the core developers to personally enforce style.

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

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

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

Do not agree. Switching to two-spaces indentation is imho very bad for readability, makes it kind of annoying to detect at which level of indent you are after a few lines, this is not visually obvious anymore… And this is a very poor band-aid for the actual issue. TBH, I’d rather enforce always putting opening brace on newline, would make files a tad lengthier, but still much better than current clang code-style "fix" on that matter. And then no need to switch to horrible two-spaces indent!

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

Also another bad thing about two-spaces indent: it encourages people putting even more levels of sub-blocks into a single function (since they do not 'cost' much space anymore), which is typically bad practice. The bigger the indent, the more obvious it becomes when you go beyond reasonable level (which is something like 4 or 5 in most cases I think…).

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

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

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

Do not agree. Switching to two-spaces indentation is imho very bad for readability, makes it kind of annoying to detect at which level of indent you are after a few lines, this is not visually obvious anymore… And this is a very poor band-aid for the actual issue. TBH, I’d rather enforce always putting opening brace on newline, would make files a tad lengthier, but still much better than current clang code-style "fix" on that matter. And then no need to switch to horrible two-spaces indent!

Probably we agree. I was only agreeing that it solves the spesific problem mentioned to begin with, not advocating for 2 space indentation, although I don't have a strong opinion - especially if we're going for 80 char width limit.

+1 for braces always on newlines, as an alternative.

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

+1

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

It's not clear to me whats being proposed now, it seems we'll have to make some compromises. Ok, which ones?

One sticking point is a fairly obvious problem with struct alignment which I reported and have no reply for 8 months.

One proposed solution is to workaround this by setting BreakBeforeBinaryOperators: All which has much wider style implications which IMHO make code less readable (code which has nothing to do with struct declarations).

We can just accept this too, but it's not great that we have to accept too many trade-offs, especially if - for all we know, they get fixed in the next release.

While on one hand, I'm happy to see that this issue is not closed with "no way!" yet, I'm also sad that there still isn't a resolution after more than a year. There is a strong dollars-and-cents case for automatic formatting, which is why so many companies have adopted it. For a volunteer project such as Blender, where core developer time is incredibly valuable, and onboarding new developers is critical to the lifeblood of the project, I hoped it would be clear that adopting something like this proposal is worthwhile. While there are edge cases that may require some compromise or a sprinkling of /*clang-format: off */, it's important to not over-weight the small up-front cost of this versus the high and ongoing tax paid by the core developers to personally enforce style.

Agree with your sentiment here.

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

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

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

Suggest either of these:

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

Notes:

  • comments and ForEachMacros in .clang-format above need updating.
  • /* multi-line-code-blocks */ will need a one time manual fix since they aren't getting properly re-indented (unless clang-format has a way to handle it).
  • We'll need to add trailing commas to all struct declarations.

+1 for 4 char indent and conditional braces on new line.

We could relax the rule of always requiring conditional braces, and only require them if them if the condition does not fit on a single line? Would gain back a bit of space, and with auto formatting you can't accidentally indent wrong.

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

Also think this could be added:

AllowShortFunctionsOnASingleLine: Inline

ForEachMacros should have 'foreach' as used in Cycles.

+1 for 4 char indent and conditional braces on new line.

We could relax the rule of always requiring conditional braces, and only require them if them if the condition does not fit on a single line? Would gain back a bit of space, and with auto formatting you can't accidentally indent wrong.

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

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

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

Also think this could be added:

AllowShortFunctionsOnASingleLine: Inline

ForEachMacros should have 'foreach' as used in Cycles.

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

Also committed trailing commas to workaround strange struct indentation rBe305560f13c3c67b048dc3889c7797eae2e345c4

Also +1 for the 4 char version.

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

The ones with BEGIN/END are not concerned here I guess? Like FOREACH_PCHAN_, FOREACH_COLLECTION_, FOREACH_SCENE_, etc.

Regarding width, am not fully against 80 chars, but I think even 100 chars should be more than enough short for side-by-side display on any modern screen? At least that’s what I can do, without even occupying the whole screen width, on my tiny laptop FullHD screen. ;) So that’s a compromise which sounds reasonable to me.

Updated P884 - 100 width (wrap at 99), added LISTBASE_FOREACH, although there are probably 20+ looping macros to add here.

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

Keir Mierle (keir) added a comment.EditedSun, Jan 6, 7:18 PM

I updated P884 and P885 to turn on IndentPPDirectives, which is now commonly available in deployed Clang versions (available as of Clang 6). This improves preprocessor formatting.

I am not in a favour of using any of *OnASingleLine family of settings enabled. They are making code more difficult for debugging (and to my knowledge even goes against existing rule of not putting things ono a single line).

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

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

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

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

I am not in a favour of using any of *OnASingleLine family of settings enabled. They are making code more difficult for debugging (and to my knowledge even goes against existing rule of not putting things ono a single line).

I'm fine with None if you prefer typical getter/setters in class bodies to be on multiple lines, I don't feel strongly about that. Mainly I wanted to change it from the default All.

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

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

Looks like clang-tidy may be able to do this , not ideal, but we'd have to run it once during migration to the new rules so it may not be that bad...

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

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

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

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

Not keen on moving away from using braces (where braces use too much vertical space there is some incentive not to use them).

This brings back the annoyance of lines from the statement and it's body running into each other, eg:

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

Becomes:

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

I suppose maintainers of each area can choose when to do this as they do now.

We would need to use do {...} while (0) in macros instead of {...} ((void)0) if we want to use them w/o braces in conditionals.

I am not in a favour of using any of *OnASingleLine family of settings enabled. They are making code more difficult for debugging (and to my knowledge even goes against existing rule of not putting things ono a single line).

I'm fine with None if you prefer typical getter/setters in class bodies to be on multiple lines, I don't feel strongly about that. Mainly I wanted to change it from the default All.

Don't feel strongly either fine w/ None, or Inline (edited P884).

*edit, IndentPPDirectives now works*

In general P884 LGTM (accepting there are some less than ideal trade-offs we can't avoid).

I didn't check the output in detail for C++ code, so I'm assuming others are OK with this configuration.

Any remaining blocking issues?


Otherwise, next steps.

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

@Brecht Van Lommel (brecht),

I'm fine with None if you prefer typical getter/setters in class bodies to be on multiple lines, I don't feel strongly about that. Mainly I wanted to change it from the default All.

Using explicit getters/setters for a trivial assignment (without any sanity checks i.e.) is often turns out to be more of a burden than helpful in comparison with direct access. But this is another discussion anyway.

@Campbell Barton (campbellbarton), is just source/ with a selective directories in intern/, like, guardedalloc. intern/ only means that it's an external library maintained by Blender developer. There is no need to enforce Blender's code style globally there.

Committed branch temp-clang-format w/ initial migration scripts, see: https://developer.blender.org/diffusion/B/browse/temp-clang-format/

These files will move, to try it out, files added are:

  • .clang-format (from P884)
  • clang-format-migration.sh converts tabs to spaces before re-formatting.
  • clang-format-edited.sh runs on modified files only.
  • Global conversion from tabs to spaces could backfire (strings that contain non-literal tabs for eg) We could replace all tabs in strings with \t literal before applying clang-format.
  • Current migration breaks makesdna by splitting:
char   vgroup[64];  /* ... long comment... */

into

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

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

Otherwise, next steps.

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

This is great! Nice progress.

@Campbell Barton (campbellbarton) - Why the need for the custom replacement of tabs with spaces in Python? clang-format handles that natively.
@Dalai Felinto (dfelinto) - Agreed there will be a long tail to this migration. First step is just to get to a .clang-format that we mostly agree on, and we're nearly there!

Regarding the makesdna issue: I have a suggestion: switch the SDNA coding standard to put comments on the line before. This fixes the formatting problem and also encourages longer and more detailed comments. A side note, my personal feeling is that not all code is created equal; some code is more core to a product/project then others, and so deserves a very high level of variable naming and commenting rigour. In my opinion, Blender's SDNA is in this category and deserves to have the most obnoxiously high quality code standards, including naming and in-code documentation (comments).

Existing:

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

Suggested:

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

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

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

The clang-format result now builds.

The reason to use expand tabs before running clang-format is clang-format isn't touching comment blocks after the first line (unless I miss some option to tell it to expand tabs but otherwise not re-wrap them).

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

You no longer need to grab llvm to get clang-format on windows, visual studio 2017 ships with clang-format support out of the box, the ide will detect the file and use it for it's auto formatting support. However given we currently have both a .clangformat (which says use spaces) and a .editorconfig (which says use tabs, also missing the .cpp file type btw) it's acting kinda bizarre, it inserts tabs, but when you manually run code formatting it changes them to spaces.

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

Why do we have ConstructorInitializerIndentWidth: 0 ? Personally i find this hard to follow (and i'm one the few who spends a lot of time in C++ parts of Blender..)

Can we have at least single, or double indentation for it?

@Sergey Sharybin (sergey), I'm fine if ConstructorInitializerIndentWidth is changed. Probably it was set like that to follow existing code style.

Running clang-format on .glsl and .osl code seems to work. I've enabled it in temp-clang-format.

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

Some other points:

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

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

Added D4185 some TODO's noted: make format can be used to try out formatting.

Clang detects header guards which mostly works, but there is a case it fails.

If this is anywhere in the header, indent guard detection fails.

#define __func__ __FUNCTION__

Applies to:

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

I've added most intern/ modules for formatting now in temp-clang-format, including Cycles. The only excluded modules still are libmv, numaapi (using Google style) and elbeem, smoke, itasc (originated outside of Blender).

This is still up for discussion, just my guess for what the relevant maintainers would want.

@Brecht Van Lommel (brecht), opensubdiv's C-API is also following google's style.

Ok, I've excluded opensubdiv as well now.

One nifty thing: git-clang-format from [1].

From a quick tests it does reformat on modified hunks (the files are to be staged first). Seems easy to distribute, and probably something to encourage people to setup as a pre-commit hook?

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

Added tests/gtest, removed BLI_ressource_strings.h since it's a datafile.

Also removed BLI_compiler_typecheck.h & BLI_utildefines_variadic.h - since results aren't acceptable and not something we're likely to be able to tweak.

One nifty thing: git-clang-format from [1].

From a quick tests it does reformat on modified hunks (the files are to be staged first). Seems easy to distribute, and probably something to encourage people to setup as a pre-commit hook?

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

+1, we'll need to filter out files that use too much memory though ./intern/cycles/render/sobol.h hangs on my system, even when using // clang-format off.

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

Update:

  • Added FixNamespaceComments ~ seems generally helpful.
  • Use clang-format off instead of excluding files (except for sobol.h).

I don't see any blockers for migrating to clang-format, besides more general docs, pre-commit hooks etc. which we might want to have before migrating.


Having said this, we might want to disable formatting in more files based on closer inspection of the output.

./source/blender/nodes/NOD_static_types.h wraps some lines and not others in a way that doesn't read well.

snippet:

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

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

Eg:

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

Becomes:

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

We could let clang-format wrap comments, making sure all ascii diagrams don't exceed the line length before formatting - or disable formatting when we can't edit to fit the width, see: ./source/blender/compositor/COM_compositor.h.