First attempt of code formatting in the Generators

First attempt of code formatting in the Generators

The used command : "astyle --keep-one-line-blocks"

Tag: @jchapman @ewelina

This merge request affects 1 package:

• Generators/McEventSelector

This merge request affects 8 files:

• Generators/McEventSelector/McEventSelector/McAddress.h
• Generators/McEventSelector/McEventSelector/McCnvSvc.h
• Generators/McEventSelector/McEventSelector/McEventCnv.h
• Generators/McEventSelector/McEventSelector/McEventSelector.h
• Generators/McEventSelector/src/McAddress.cxx
• Generators/McEventSelector/src/McCnvSvc.cxx
• Generators/McEventSelector/src/McEventCnv.cxx
• Generators/McEventSelector/src/McEventSelector.cxx

added Generators master review-pending-level-1 labels

The system determined that CI tests (with names matching "^CITest_Trigger.*$") are not needed for this code change. They are not run. This is not an indicator to restart the job.

CI Result FAILURE (hash f52fcc2c)

	Athena	AthSimulation	AthGeneration
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
For experts only: Jenkins output [CI-MERGE-REQUEST-CC7 62553]

removed review-pending-level-1 label

added review-pending-expert label

This looks like an experiment . I leave to @jchapman and @ewelina to decide if it worths it or not

I'm in favour of trying to make a common formatting work. At least for some areas in our code.

Generators may actually be a perfect testbed for this. Since as far as I understand there is a lot less need for sweeps in generator code than for most other code areas.

If anything, I would go even further with these updates. This MR only makes some indentation changes. (And removes a bunch of unnecessary line-ending spaces.) But it doesn't even remove unnecessary double-spaces in the middle of lines. So I would still be more in favour of clang-format. Which is capable of a lot more thorough formatting. (And which we use in all of the Acts projects for instance.)

But if indentation changes are the only things that we feel brave enough for (I'm provocative on purpose here... ) then as far as I'm concerned, we should try this.

Hi @akraszna ,

1. thank you for the support.

2. Yes, this is just a test that we discussed with @jchapman privately and we will have to look at how brave we are.

3. So I would still be more in favour of clang-format.

Me too, if to go on a larger scale. Moreover, with this MR there was an expectation that someone would suggest "an officially approved clang-format config file for ATLAS". Would be great if you have any or if it makes sense to borrow it from ACTS. But, so far the astyle is a poor man's option. Basically, it does remove useless spaces and that is already good.

Best regards,

Andrii

Hi

quite some time ago I created that [https://gitlab.cern.ch/ATLAS-EGamma/Software/Reconstruction/clang-tidy-checks] Actually the clang-tidy has been more "usefull" in certain contexts than the format but this is another story for another day.

For the format see : [https://gitlab.cern.ch/ATLAS-EGamma/Software/Reconstruction/clang-tidy-checks/-/tree/master/clang_format_templates]

these were created by this kind of command for some other discussion (that went nowhere)

clang-format -style=llvm -dump-config > .clang-format

The good thing for .clang-format is that one can use it also from vi and emacs which a lot of us use (will not disclose my "church here) . Then I picked a variant of Mozilla (llvm also made sense, linux is a bit weird for C++ nice for C) and use it but I never run on bot mode, I use it when I really clean up code, from my editor, refactor. So for a team that works on a small code and wants to be self consistent you can use it and go as you refactor no?

The issue I left this story ,which might not be an issue for a local package, is that there is not a single style-guide > 120 and only one at 100 wide for the ColumnLimit which makes sense if you have ever used a 13 inc laptop and have tried to browse out code in gitlab with the side panel on.

A lot of our code would be fine with 80 which is what most style guides suggest. In the other hand there is at least one major domain that uses 140 in ATLAS and some strong opionions on 120 .

At the end is prb not that importand but I do not know if we can have "ATLAS" wide bot re-formatting. We can have some suggestion I guess for now , and ?i think one can put one in a domain .

The one good things (the following is from @ssnyder since d0 time it seems ? )

(defun d0-lineup-comment (langelem)
 (if (or (assq 'ansi-funcdecl-cont c-syntactic-context)
         (assq 'func-decl-cont c-syntactic-context)
         (assq 'knr-argdecl-intro  c-syntactic-context))
     -1000
   (c-lineup-comment langelem)))

(c-add-style "d0"
            '("gnu" ; similar to gnu
              (c-offsets-alist . ((substatement-open . 0)
                                  (comment-intro . d0-lineup-comment)
                                  (innamespace . 0)
                                  (member-init-cont . 0)
                                  ))
              (c-label-minimum-indentation . 0)
              ))

is that most of the standard will not indent on namespace etc. I know from a common file that there is slight difference on macro spacing but other than that is close.

Also let me ping @tadej

mentioned in merge request !58781 (merged)

Hi @christos ,

1. Good that we have this discussion.
2. There are many experiences and viewpoints at the line widths and the optimal sizes of the laptop screens. So, well, that makes things more complex.
3. I've applied the clang-format style from @akraszna and it looks like overkill for me. At least at this point.
4. Given 2) and 3) I think that it makes sense to leave this MR as it is. The astyle with the used settings is really not intrusive and does just a bare minimum of formatting. Good start, as for me.

Best regards,

Andrii

added 7 commits

• f52fcc2c...0b4e418f - 6 commits from branch atlas:master
• f98f9310 - Merge remote-tracking branch 'upstream/master' into generator_formating_1

Compare with previous version

Build area was cleaned as per request posted in the DB. The full software build will be performed

This merge request affects 1 package:

• Generators/McEventSelector

This merge request affects 8 files:

• Generators/McEventSelector/McEventSelector/McAddress.h
• Generators/McEventSelector/McEventSelector/McCnvSvc.h
• Generators/McEventSelector/McEventSelector/McEventCnv.h
• Generators/McEventSelector/McEventSelector/McEventSelector.h
• Generators/McEventSelector/src/McAddress.cxx
• Generators/McEventSelector/src/McCnvSvc.cxx
• Generators/McEventSelector/src/McEventCnv.cxx
• Generators/McEventSelector/src/McEventSelector.cxx

added review-pending-level-1 label and removed review-pending-expert label

The system determined that CI tests (with names matching "^CITest_Trigger.*$") are not needed for this code change. They are not run. This is not an indicator to restart the job.

CI Result FAILURE (hash f98f9310)

	Athena	AthSimulation	AthGeneration
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
For experts only: Jenkins output [CI-MERGE-REQUEST-CC7 62596]

The failure are unrelated (ATLASRECTS-7384)

I agree with @averbyts. Let us keep this MR as it is. We have one package with minimal formatting which may be used as an example for a discussion on the GIT meeting about the proposal for code formatting in Generators.