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 configuration #1920

Merged
merged 1 commit into from
Jun 19, 2020
Merged

Clang-format configuration #1920

merged 1 commit into from
Jun 19, 2020

Conversation

offa
Copy link
Contributor

@offa offa commented Apr 29, 2020

Description

Initial .clang-format file, open for discussion. The config doesn't contain much as the defaults fit most cases already. Documentation of all options is available here.

Settings to discuss / change

  • NamespaceIndentation: Indention of namespaces – All, Inner or None?
  • IndentPPDirectives: Indention of proprocessor directives

GitHub Issues

#1182

@codecov
Copy link

codecov bot commented Apr 29, 2020

Codecov Report

Merging #1920 into master will increase coverage by 1.80%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1920      +/-   ##
==========================================
+ Coverage   86.80%   88.59%   +1.80%     
==========================================
  Files         131      138       +7     
  Lines        3923     5619    +1696     
==========================================
+ Hits         3405     4978    +1573     
- Misses        518      641     +123     

@horenmar
Copy link
Member

Right, I'll try to find some time to test the configuration out.

However, since opening #1182, I started having some misgivings about clang-format. From my experience with other projects, it doesn't handle some constructs well, and the actual formatting depends on the specific version of clang-format, making inconsistencies likely.

@offa
Copy link
Contributor Author

offa commented May 19, 2020

Please let me know if there are some issues. The result might not be perfect, but maybe we can get some consistency.

@horenmar
Copy link
Member

So, I spent some time playing with the config and made some further changes (pushed them as a separate commit). There is still at least one problem that needs fixing before I am willing to say "use clang-format now". See below

Some notes on the current settings

  • NamespaceIndentation - I would love a hybrid between Inner and All, where the most-nested namespace is indented, but the less nested ones aren't. That doesn't exist, so I ended up going with All.
  • IndentPPDirectives - This is currently inconsistent in the codebase, but AfterHash is the style I prefer for new code.
  • AllowShortEnumsOnASingleLine - preferably would be set to False, but that seems to be a clang-format-11 thing.

The big problem is with SpacesInParentheses. I don't care for it that much personally, but the absolute majority of current code is written in style that is closely resembled by using it, but not quite. The problems come from parenthesized expressions (such as defensively written macro argument expansion), which aren't either function decl or function call.

I think I will end up turning it on and trying it out a bit more, because I don't want to generate mostly pointless diff against all code.

@offa
Copy link
Contributor Author

offa commented Jun 15, 2020

AllowShortEnumsOnASingleLine - preferably would be set to False, but that seems to be a clang-format-11 thing.

I've just check the clang docs, you are right, clang-format 10 doesn't mention it.

The problems come from parenthesized expressions (such as defensively written macro argument expansion), which aren't either function decl or function call.

I see, in the worst case it might be an option to disable clang format for particular sections.

I think I will end up turning it on and trying it out a bit more, because I don't want to generate mostly pointless diff against all code.

👍

Some notes on the configuration options chosen:

* We want `AllowShortEnumsOnASingleLine` set to `false`, but that
option is clang-format-11 and up, which is not out yet.
* `IndentPPDirectives` is currently inconsistent, but `AfterHash`
is the preferred style in new code.
* `NamespaceIndentation` is a mess, but `All` is closer to the effect
we want than `Inner`.
* `SpacesInParentheses` set to `true` is not ideal due to it also
introducing extra spaces in preprocessor expressions, but using it
is much closer to the current style than not.

All in all, using this setting globally would reformat pretty much
every line of code in the codebase, but it is as close as possible
to the bespoke style currently used. Still, it should only be used
on the diffs.

Closes catchorg#1182
@horenmar horenmar merged commit e815acd into catchorg:master Jun 19, 2020
@offa offa deleted the clangformat branch June 19, 2020 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants