Policy to change clang-format brace placement #75956

New Issue

Campbell Barton · 2020-04-21T10:01:17+02:00

Campbell Barton commented

2020-04-21 10:01:17 +02:00

Note, this proposal is for when we switch to clang-format 10, which is probably not something that's expected to happen soon.

This task proposes to use clang-format with multi-line brace placement.

This was used for Blender's code-style before using clang-format, however clang-format didn't support differentiating between multi-line and single-lines.

With the release of clang-format 10, this is now supported.

Motivation

Better readability by separating checks from if statements and loops with the body of code block.

Example:

Before:

  if (ELEM(event->type, MOUSEMOVE, INBETWEEN_MOUSEMOVE) ||
      /* if we started dragging, progress on any event */
      (data->multi_data.init == BUTTON_MULTI_INIT_SETUP)) {
    if (ELEM(but->type, UI_BTYPE_NUM, UI_BTYPE_NUM_SLIDER) &&
        ELEM(data->state, BUTTON_STATE_TEXT_EDITING, BUTTON_STATE_NUM_EDITING)) {
      /* initialize! */
      if (data->multi_data.init == BUTTON_MULTI_INIT_UNSET) {
        /* --- snip --- */
      }
    }
  }

After:

  if (ELEM(event->type, MOUSEMOVE, INBETWEEN_MOUSEMOVE) ||
      /* if we started dragging, progress on any event */
      (data->multi_data.init == BUTTON_MULTI_INIT_SETUP))
  {
    if (ELEM(but->type, UI_BTYPE_NUM, UI_BTYPE_NUM_SLIDER) &&
        ELEM(data->state, BUTTON_STATE_TEXT_EDITING, BUTTON_STATE_NUM_EDITING))
    {
      /* initialize! */
      if (data->multi_data.init == BUTTON_MULTI_INIT_UNSET) {
        /* --- snip --- */
      }
    }
  }

Before:

    if (!_PyArg_ParseTupleAndKeywordsFast(args,
                                          kw,
                                          &_parser,
                                          &id,
                                          &id_len,
                                          &name,
                                          &description,
                                          PyC_ParseBool,
                                          &def,
                                          &PySet_Type,
                                          &pyopts,
                                          &PySet_Type,
                                          &pyopts_override,
                                          &PySet_Type,
                                          &py_tags,
                                          &pysubtype,
                                          &update_cb,
                                          &get_cb,
                                          &set_cb)) {
      return NULL;
    }

After:

    if (!_PyArg_ParseTupleAndKeywordsFast(args,
                                          kw,
                                          &_parser,
                                          &id,
                                          &id_len,
                                          &name,
                                          &description,
                                          PyC_ParseBool,
                                          &def,
                                          &PySet_Type,
                                          &pyopts,
                                          &PySet_Type,
                                          &pyopts_override,
                                          &PySet_Type,
                                          &py_tags,
                                          &pysubtype,
                                          &update_cb,
                                          &get_cb,
                                          &set_cb))
    {
      return NULL;
    }

Before:

        if (CustomData_data_equals(
                lwc->type, lwc->data_ref, BM_ELEM_CD_GET_VOID_P(l_other, lwc->cd_layer_offset))) {
          bm_loop_walk_data(lwc, l_other);
        }

After:

        if (CustomData_data_equals(
                lwc->type, lwc->data_ref, BM_ELEM_CD_GET_VOID_P(l_other, lwc->cd_layer_offset)))
        {
          bm_loop_walk_data(lwc, l_other);
        }

Tradeoff's

Pros

Improved readability for blocks of code which often end up being poorly separated in cases where the multi-line statements are already becoming hard to follow.

Cons

It's a disruptive change for branches.
Uses more vertical space.

Proposal

Enable this option once we've updated to clang-format version 10.

Change to .clang-format (with surrounding values omitted).

BraceWrapping: {
    AfterControlStatement: 'MultiLine' 
}

*Note, this proposal is for when we switch to clang-format 10, which is probably not something that's expected to happen soon.* ---- This task proposes to use clang-format with multi-line brace placement. This was used for Blender's code-style before using clang-format, however clang-format didn't support differentiating between multi-line and single-lines. With the release of clang-format 10, this is now supported. **Motivation** Better readability by separating checks from `if` statements and loops with the body of code block. ---- Example: Before: ``` if (ELEM(event->type, MOUSEMOVE, INBETWEEN_MOUSEMOVE) || /* if we started dragging, progress on any event */ (data->multi_data.init == BUTTON_MULTI_INIT_SETUP)) { if (ELEM(but->type, UI_BTYPE_NUM, UI_BTYPE_NUM_SLIDER) && ELEM(data->state, BUTTON_STATE_TEXT_EDITING, BUTTON_STATE_NUM_EDITING)) { /* initialize! */ if (data->multi_data.init == BUTTON_MULTI_INIT_UNSET) { /* --- snip --- */ } } } ``` After: ``` if (ELEM(event->type, MOUSEMOVE, INBETWEEN_MOUSEMOVE) || /* if we started dragging, progress on any event */ (data->multi_data.init == BUTTON_MULTI_INIT_SETUP)) { if (ELEM(but->type, UI_BTYPE_NUM, UI_BTYPE_NUM_SLIDER) && ELEM(data->state, BUTTON_STATE_TEXT_EDITING, BUTTON_STATE_NUM_EDITING)) { /* initialize! */ if (data->multi_data.init == BUTTON_MULTI_INIT_UNSET) { /* --- snip --- */ } } } ``` Before: ``` if (!_PyArg_ParseTupleAndKeywordsFast(args, kw, &_parser, &id, &id_len, &name, &description, PyC_ParseBool, &def, &PySet_Type, &pyopts, &PySet_Type, &pyopts_override, &PySet_Type, &py_tags, &pysubtype, &update_cb, &get_cb, &set_cb)) { return NULL; } ``` After: ``` if (!_PyArg_ParseTupleAndKeywordsFast(args, kw, &_parser, &id, &id_len, &name, &description, PyC_ParseBool, &def, &PySet_Type, &pyopts, &PySet_Type, &pyopts_override, &PySet_Type, &py_tags, &pysubtype, &update_cb, &get_cb, &set_cb)) { return NULL; } ``` Before: ``` if (CustomData_data_equals( lwc->type, lwc->data_ref, BM_ELEM_CD_GET_VOID_P(l_other, lwc->cd_layer_offset))) { bm_loop_walk_data(lwc, l_other); } ``` After: ``` if (CustomData_data_equals( lwc->type, lwc->data_ref, BM_ELEM_CD_GET_VOID_P(l_other, lwc->cd_layer_offset))) { bm_loop_walk_data(lwc, l_other); } ``` **Tradeoff's** **Pros** - Improved readability for blocks of code which often end up being poorly separated in cases where the multi-line statements are already becoming hard to follow. **Cons** - It's a disruptive change for branches. - Uses more vertical space. ---- **Proposal** Enable this option once we've updated to clang-format version 10. Change to `.clang-format` (with surrounding values omitted). ``` BraceWrapping: { AfterControlStatement: 'MultiLine' } ```

Campbell Barton self-assigned this 2020-04-21 10:01:17 +02:00

Campbell Barton commented

2020-04-21 10:01:17 +02:00

Changed status from 'Needs Triage' to: 'Confirmed'

Campbell Barton commented

2020-04-21 10:01:17 +02:00

Added subscriber: @ideasman42

Sergey Sharybin commented

2021-02-11 18:59:08 +01:00

Added subscriber: @Sergey

Sergey Sharybin commented

2021-02-11 18:59:08 +01:00

I see this as a bandage on top of a improperly structured code, and the improved readability is subjective here. The more readable version would be if (need_initialize_multi_data(...)) and move all those levels of nesting into a function, sue early returns, keep indentation low, describe conditions as needed. And keep the current formatting, so that developers are disguised and are forced to type properly readable code ;)

To me this is a disruptive change, which doesn't solve the claimed root issue.

I see this as a bandage on top of a improperly structured code, and the improved readability is subjective here. The more readable version would be `if (need_initialize_multi_data(...))` and move all those levels of nesting into a function, sue early returns, keep indentation low, describe conditions as needed. And keep the current formatting, so that developers are disguised and are forced to type properly readable code ;) To me this is a disruptive change, which doesn't solve the claimed root issue.

Sergey Sharybin commented

2021-02-11 19:36:00 +01:00

Multi-post spam, yay!

Had a quick chat with Dalai about how to make such reviews don't feel difficult for any of the sides. To come to the consensus, here is my statement: I don't find this proposal great, but I can accept it.

Multi-post spam, yay! Had a quick chat with Dalai about how to make such reviews don't feel difficult for any of the sides. To come to the consensus, here is my statement: I don't find this proposal great, but I can accept it.

Bastien Montagne commented

2021-02-17 11:54:25 +01:00

Added subscriber: @mont29

Bastien Montagne commented

2021-02-17 11:54:25 +01:00

Big +1 from me, that was one of the main issues I had when we switched to Clang-format.

Campbell Barton commented

2021-02-17 12:22:04 +01:00

+1, even though the code that reads poorly could be refactored in some cases. There is enough of this that can't/wont be easily changed.

Brecht Van Lommel commented

2021-02-17 17:52:01 +01:00

Added subscriber: @brecht

Brecht Van Lommel commented

2021-02-17 17:52:01 +01:00

I have no preference, either is fine.

Campbell Barton referenced this issue

2023-04-21 07:22:49 +02:00

clang-format: set BraceWrapping::AfterControlStatement to "MultiLine" #107207

Sybren A. Stüvel commented

2023-05-01 11:35:40 +02:00

I agree with @Sergey, both on the 'this should be refactored' front and the 'I can live with this' front.

Campbell Barton referenced this issue from a commit

2023-05-02 02:50:59 +02:00

clang-format: set BraceWrapping::AfterControlStatement to "MultiLine"

Campbell Barton commented

2023-05-02 03:01:47 +02:00

Applied:

.clang-format: 391f86bc38
make format: 6859bb6e67
manual cleanup afterwards: a0db0a5580

Applied: - `.clang-format`: 391f86bc387d745de897503c36d11cd65691b84c - `make format`: 6859bb6e67031765e79e525ae62bf2ebf4df2330 - manual cleanup afterwards: a0db0a55804eb396d4a19dd5f692dd7e9eebb81d

Campbell Barton closed this issue

2023-05-02 03:01:49 +02:00

Blender Bot added

and removed

labels 2023-05-02 03:01:51 +02:00

Sign in to join this conversation.