Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

clang-format config changes to discuss #29848

Closed
wants to merge 2 commits into from

Conversation

akien-mga
Copy link
Member

@akien-mga akien-mga commented Jun 17, 2019

I tried two changes to our clang-format config which make it stricter, but should also enforce a style that we are more or less using already, with exceptions.

  • AllowShortCaseLabelsOnASingleLine: If true, short case labels will be contracted to a single line.
  • AllowShortIfStatementsOnASingleLine: If true, if (a) return; can be put on a single line.

Edit: To be clear, the proposed changes is to disable both of those, so disallow short case labels and if statements on a single line.

See https://clang.llvm.org/docs/ClangFormatStyleOptions.html

I only open this PR for discussion, I'm not necessary convinced that we should do this change, but it's worth discussing.

@fire
Copy link
Member

fire commented Jun 19, 2019

I noticed sometimes

if (a) return;

and

if (a)
  return;

is used.

I would prefer

if (a) {
  return;
}

However, using braces is a massive style change.

Using braces is better because the braces allow future modification without changing the style.

Thoughts?

@aaronfranke
Copy link
Member

aaronfranke commented Jun 19, 2019

For simple statements, I think it looks fine to have:

if (a) return;

But I would prefer to explicitly include braces:

if (a) {
    return;
}

Over not doing so:

if (a)
    return;

While I know this is C++ and not C# being discussed, this issue is relevant: godotengine/godot-docs#2504

@lawnjelly
Copy link
Member

Isn't the usual reason given to prefer if statements over more than one line ease of debugging?

* if (a) return;
if (a)
    * return;

Where * represents a breakpoint. With the latter you can break on the condition.

Personally I'm fine with the non-braced version, as it makes things less verbose so you can fit more on screen (and less of those clang errors 😸 ). But is a matter of taste.

@akien-mga
Copy link
Member Author

I'll close this for now and come back to it after 3.2, these changes should not be done before merging the vulkan branch into master anyway.

@akien-mga akien-mga closed this Sep 3, 2019
@akien-mga akien-mga deleted the stylé-comme-jamais branch September 3, 2019 09:04
@akien-mga
Copy link
Member Author

We actually agreed to apply both those changes at the Godot Sprint with several core contributors, so I'll make another PR once vulkan has been merged with master.

akien-mga added a commit to akien-mga/godot that referenced this pull request May 10, 2020
Part of godotengine#33027, also discussed in godotengine#29848.

Enforcing the use of brackets even on single line statements would be
preferred, but `clang-format` doesn't have this functionality yet.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants