search

dune-community / dune-xt-common

Other
2 stars 3 forks source link

Update clang-format spec/binary #147

Closed renefritze closed 5 years ago

renefritze commented 5 years ago

It would be very beneficial to my workflow if we could agree to bump the clang-format version to 6. Version 3.9 is no longer available in debian unstable. The minimum there is 6. 6 is also available for Stretch via backports. I'm hoping getting 6 on arch isn't problematic either. Sadly applying our 3.9 config with a clang-format-6 introduces new changes. I would look into whether we can adjust the config to prevent this or if we'd have to accept those if @dune-community/dune-xt-devs are ok with bumping the version in principle.

renefritze commented 5 years ago

@ftalbrecht said Arch should not be an issue for clang-format-6 (aside from the docker setup being broken itself atm) so I'll be looking into what changes we can make for a no-diff-apply with the newer format binary today

renefritze commented 5 years ago

There's good and bad news. Some stuff like putting 'empty' braces on a single line got much better. Operator alignment got much worse, but only in some cases and I do not understand the differences. Indent/continuation for multi-inheritance, initializer got (I think) more consistent with the rest of the code. Here's the updated spec: https://github.com/dune-community/vcsetup/compare/clang_format_6 And the changes introduced in source, split roughly by category: https://github.com/dune-community/dune-xt-common/compare/clang_format_6

renefritze commented 5 years ago

See also: https://github.com/dune-community/dune-xt-la/compare/clang_format_6

ftalbrecht commented 5 years ago

I welcome some of these changes (empty curly bracse + operator alignment, in particular regarding <<), the others I can well live with. Go ahead and implement this everywhere, if you want to. Any idea on how to do this smoothly, given nontrivial changes and several branches and the like. Only do this on the new master?

renefritze commented 5 years ago

Yeah, I would only do this after all the 'new masters' got pushed. Operator alignment is still bugging me though. Maybe I'll check an even newer clang-format. Maybe it's a bug after all.

renefritze commented 5 years ago

There's also some new options we might want to consider, @dune-community/dune-xt-devs :

FixNamespaceComments (bool) If true, clang-format adds missing namespace end comments and fixes invalid existing ones.

I think enabling this is a no-brainer

CompactNamespaces (bool) If true, consecutive namespace declarations will be on the same line. If false, each namespace is declared on a new line.

I like this also.

IndentPPDirectives (PPDirectiveIndentStyle) The preprocessor directive indenting style to use.

IIRC before we started using clang-format we manually indented preprocessor directives (in if blocks for example). This setting would bring that back.

Spec file

ftalbrecht commented 5 years ago

On November 14, 2018 9:29:53 AM GMT+01:00, "René Fritze" notifications@github.com wrote:

There's also some new options we might want to consider:

FixNamespaceComments (bool) If true, clang-format adds missing namespace end comments and fixes invalid existing ones.

I think enabling this is a no-brainer

CompactNamespaces (bool) If true, consecutive namespace declarations will be on the same line. If false, each namespace is declared on a new line.

I like this also.

This one I dont like that much. The others I am fine with.

IndentPPDirectives (PPDirectiveIndentStyle) The preprocessor directive indenting style to use.

IIRC before we started using clang-format we manually indented preprocessor directives (in if blocks for example). This setting would bring that back.

Spec file

renefritze commented 5 years ago

closed by #151