Clang Format Configuration, master branch (2022.11.28.)

This merge request affects no known packages. Consult an expert!

This merge request affects 1 file:

.clang-format

Could not map the following files to a domain:

.clang-format

added master no-domain review-pending-level-1 labels

Also tagging @jojungge

approved this merge request

Hi

I think this needs a discussion where you actually take something that is well formatted and you show what it will happen with the 3 main editors or a bot (I assume they will do the same)

Also we will need to advertise how this is to be used from the other 2 editors I know people use. As I said I use it a lot (really a lot) from one of them ....

removed review-pending-level-1 label

marked this merge request as draft

Hi @akraszna ,

Thank you!
Could you, please, remind me what would be the range of clang-format versions that one can actually use? If I'm not wrong there might be small differences in the ways the clang-format interprets the config.
Maybe just add a couple of lines on the formatting and how to use this config to the documentation. The manual for shifters?

Best regards,

Andrii

It depends how one setups ATLAS .... release since we have clang releases.

For both tidy and format I do

source  /cvmfs/atlas.cern.ch/repo/ATLASLocalRootBase/x86_64/Clang/14.0.0-x86_64-centos7-gcc11-opt/14.0.0/x86_64-centos7-gcc11-opt/setup.sh

and then it does not matter even if I compile with gcc I can run the tidy

Btw you are going a bit too fast again.

anyhow look my points here and in the other MR.

Please take something well formatted like from CxxUtils or core and show how it changes
Then take something large (extrapolator) and show it.
i.e show what this will do to both consistently and not consistently formatted files.
If this is suggested keep in mind some of us have ones that we use and you can override it when we edit, which then breaks the "consistency" with what is there .
Prb we want it mandatory for all of us (?) but then this needs the discussion
Not everyone uses vscode we need the instruction for emacs and Vi
There are other points ....
If is not mandatory not sure why it goes to "shifters" manual?

To clarify: This MR literally does nothing to the CI, the nightly, or any source file. It just adds one small file in the repository, without any other change actually happening.

One thing that we could recommend for developers could be to run

git clang-format <source file>

before they would commit the file. So that all modifications would become formatted. This we could be able to even require in the CI if there is enough political momentum behind us.

But by default I didn't meant to re-format any of our existing code.

Let me try to spell out the behaviour a bit more. As I wrote before, this would just be the default for any source file in any package that does not override it. What I mean is like the following:

[bash][celeborn]:development > find clang-format/
clang-format/
clang-format/source.cpp
clang-format/source1
clang-format/source1/source.cpp
clang-format/source1/.clang-format
clang-format/.clang-format
clang-format/source2
clang-format/source2/source.cpp
[bash][celeborn]:development > more clang-format/.clang-format 
BasedOnStyle: Google
Language: Cpp
[bash][celeborn]:development > more clang-format/source1/.clang-format 
BasedOnStyle: Mozilla
Language: Cpp
[bash][celeborn]:development > more clang-format/source.cpp 

#include <cstring>

int func1(const char* foo) {
  if (std::strcmp(foo, "bar") == 0) {
    return 5;
  }
  return 1;
}
[bash][celeborn]:development > more clang-format/source1/source.cpp 

#include <cstring>

int
func1(const char* foo)
{
  if (std::strcmp(foo, "bar") == 0) {
    return 5;
  }
  return 1;
}
[bash][celeborn]:development > more clang-format/source2/source.cpp 

#include <cstring>

int func1(const char* foo) {
  if (std::strcmp(foo, "bar") == 0) {
    return 5;
  }
  return 1;
}
[bash][celeborn]:development >

Hopefully this clarifies a bit what the intended behaviour is. (The difference between the Google and Mozilla style is not easy to demonstrate in just a few lines.)

And to make it explicit: This configuration would modify every single source file in the repository if we were to apply it to everything. But that is not what I'm proposing.

resolved all threads

Hi

The difference for llvm, google and mozilla are minor for the basic settings.

One favor can you run

 clang-format --dump-config

and put all settings here . I have no issue with what I see this doing actually

marked this merge request as ready

On the editor discussion: VS Code, vi, and I'm pretty sure emacs and most other editors that people use can be configured to use clang-format. If you do, the native "reformat" functionality will just automatically produce compatible results. I've been using this for years in vim.

If I can make one other recommendation on the configuration is to not optimize it to look like this or that. In every configuration you will find edge cases where something doesn't look as nice, and so on. The least painful way is to pick something and just roll with it.

resolved all threads

added review-approved label

OK so the dump is [!58781 (comment 6233885)]

Let's put it in as far as I am concerned . For me anyhow is good enoguh. edge cases will happen also when changing clang-version.

The "overriding as little as possible" works for me. Just making sure that people understand the "defaults" (the ones we do not override) which for me make sense , but want them to be "clear" . There was "resistance" on one of them in the past ...

Other items :

It would be lovely if we indicate people clang to use. I would naively thing is the one we build athena with /cvmfs/atlas.cern.ch/repo/ATLASLocalRootBase/x86_64/Clang/14.0.0-x86_64-centos7-gcc11-opt/14.0.0/x86_64-centos7-gcc11-opt/setup.sh but OK
Then we have some other things to decide on when is "mandatory" where "optiona"l . How is to be applied etc. At the end can be applied via a git command. One should be careful as for sure it will change some files by a lot, for others will be "epsilon".

But these is for another time prb a meeting .

CI Result FAILURE (hash 8c655a58)

	Athena	AthSimulation	AthGeneration	AnalysisBase	AthAnalysis	DetCommon
externals
cmake
make
tests

Full details available on this CI monitor view. Check the JIRA CI status board for known problems
Athena: number of compilation errors 0, warnings 0
AthSimulation: number of compilation errors 0, warnings 0
AthGeneration: number of compilation errors 0, warnings 0
AnalysisBase: number of compilation errors 0, warnings 0
AthAnalysis: number of compilation errors 0, warnings 0
DetCommon: number of compilation errors 0, warnings 0
For experts only: Jenkins output [CI-MERGE-REQUEST-CC7 62590]

Failure obviously unrelated ... [https://its.cern.ch/jira/projects/ATLASRECTS/issues/ATLASRECTS-7384]

mentioned in merge request !58779 (merged)

merged

mentioned in commit 51d1f6a6

added sweep:ignore label

Hey coming back to this with a minor comment

it seems :

SortIncludes (SortIncludesOptions) clang-format 3.8

Controls if and how clang-format will sort #includes. If Never, includes are never sorted. If CaseInsensitive, includes are sorted in an ASCIIbetical or case insensitive fashion. If CaseSensitive, includes are sorted in an alphabetical or case sensitive fashion. Possible values:

This is fine if the include "blocks" are separated with //

but one needs to be careful vis a vis

rdering of #include statements. [include-ordering]

#include directives should generally be listed according to dependency ordering, with the files that have the most dependencies coming first. This implies that the first #include in a .cxx'' file should be the corresponding .h'' file, followed by other #include directives from the same package. These would then be followed by #include directives for other packages, again ordered from most to least dependent. Finally, system #include directives should come last.

From the style guide.

and in places where include "ordering" might matter ...

mentioned in merge request !58948 (merged)

Clang Format Configuration, master branch (2022.11.28.)

Merge request reports

Activity