[format] Use AlignFunctionDeclarations from clang-format 20.1.0

[pcanal]

Clang-format v20.1.0 and above have a new sub-option AlignFunctionDeclarations (part of the option AlignConsecutiveDeclarations) (see https://clang.llvm.org/docs/ClangFormatStyleOptions.html).

This PR was made to see how clang-formatting some of the core ROOT source would look like with it.

The alignment of functions is actually pretty close to what we expect/want.

However the enums declaration might still benefit from AlignConsecutiveAssignments but it would apply to all assignment (which has been controversial).

Ubuntu 24 does not have clang-format v20 and thus a bit a gymnastic was needed to install it.

The new sub-option is fatal to older version of clang-format, so we would all be required to use/install clang-format v20 and newer (v21 was just released on August 26).

[github-actions]

Test Results

    22 files      22 suites   3d 7h 35m 20s ⏱️
 3 834 tests  3 833 ✅ 1 💤 0 ❌
76 577 runs  76 568 ✅ 9 💤 0 ❌

Results for commit 184f47c.

♻️ This comment has been updated with latest results.

[vepadulano]

I would personally favour this move 👍

[dpiparo]

Thanks a lot for thinking this through: it is for me a good improvement. LGTM.

[silverweed]

I just tested this change on a random RNTuple header and it seems to have at least some undesirable effect, changing this:

   static std::unique_ptr<RNTupleWriter> Append(std::unique_ptr<ROOT::RNTupleModel> model, std::string_view ntupleName,
                                                TDirectory &fileOrDirectory,
                                                const ROOT::RNTupleWriteOptions &options = ROOT::RNTupleWriteOptions());

to this:

   static std::unique_ptr<RNTupleWriter> Append(std::unique_ptr<ROOT::RNTupleModel> model, std::string_view ntupleName,
                                                TDirectory                      &fileOrDirectory,
                                                const ROOT::RNTupleWriteOptions &options = ROOT::RNTupleWriteOptions());

Why is it having an effect on function parameters like this?

It seems to also have an effect on field declarations for some reason:

   RNTupleFillContext fFillContext;
   Experimental::Detail::RNTupleMetrics fMetrics;

becomes:

   RNTupleFillContext                   fFillContext;
   Experimental::Detail::RNTupleMetrics fMetrics;

I guess this is because of

AlignConsecutiveDeclarations:
  Enabled: true

If this can be limited to function declarations then it's probably ok, but I would rather not have it affect fields and parameters.

[hageboeck]

As a followup to @silverweed's comment here, it seems to also indent declarations of variables in functions in the .cxx:

 TObject *TObject::DrawClone(Option_t *option) const
 {
    TVirtualPad::TContext ctxt(true);
-   auto pad = gROOT->GetSelectedPad();
+   auto                  pad = gROOT->GetSelectedPad();
    if (pad)
       pad->cd();

I don't know if this was the desired effect?

[pcanal]

It seems to also have an effect on field declarations for some reason:

It is the intention. See for example TBuffer.h

it seems to also indent declarations of variables in functions in the .cxx:

Odd but also the intent. In both case you can still get the alternative desired effect by adding a 'semantic' empty line between the two lines. i.e. if the author does not want them aligned must mean that they are not correlated and thus do not need to read 'together' (i.e. vague explanation of the 'semantic' of the empty line):

  RNTupleFillContext fFillContext;

  Experimental::Detail::RNTupleMetrics fMetrics;

 TObject *TObject::DrawClone(Option_t *option) const
 {
    TVirtualPad::TContext ctxt(true);
+   
    auto pad = gROOT->GetSelectedPad();
    if (pad)
       pad->cd()

I just tested this change on a random RNTuple header and it seems to have at least some undesirable effect, changing this:
....

That one is not intended and looks annoying, let's see if we can somehow avoid it.

[silverweed]

it seems to also indent declarations of variables in functions in the .cxx:

Odd but also the intent.

Personally, I would be strongly against aligning local variables in functions. I find it distracting and pointless.
With class fields it's not as bad, but still too invasive imho.

[pcanal]

Why is it having an effect on function parameters like this?

Actually in example where the type name length is not as distorted, I like/prefer the clang-format proposed layout.
eg. from

   static std::unique_ptr<RNTupleWriter> Create(std::unique_ptr<ROOT::RNTupleModel> model,
                                                std::unique_ptr<Internal::RPageSink> sink,
                                                const ROOT::RNTupleWriteOptions &options);

to

   static std::unique_ptr<RNTupleWriter> Create(std::unique_ptr<ROOT::RNTupleModel>  model,
                                                std::unique_ptr<Internal::RPageSink> sink,
                                                const ROOT::RNTupleWriteOptions     &options)

where the later is in my opinion my tidy.

[silverweed]

Why is it having an effect on function parameters like this?

Actually in example where the type name length is not as distorted, I like/prefer the clang-format proposed layout. eg. from

   static std::unique_ptr<RNTupleWriter> Create(std::unique_ptr<ROOT::RNTupleModel> model,
                                                std::unique_ptr<Internal::RPageSink> sink,
                                                const ROOT::RNTupleWriteOptions &options);

to

   static std::unique_ptr<RNTupleWriter> Create(std::unique_ptr<ROOT::RNTupleModel>  model,
                                                std::unique_ptr<Internal::RPageSink> sink,
                                                const ROOT::RNTupleWriteOptions     &options)

where the later is in my opinion my tidy.

There are cases where it's better and ones where it's worse, but imho the "worse" cases degrade the situation more than how much the "better" cases improve.
It's also a bigger departure from how code has recently been layed out (even though it used to be the style in the past), so I question the benefits of this change.

[pcanal]

Personally, I would be strongly against aligning local variables in functions. I find it distracting and pointless.
With class fields it's not as bad, but still too invasive imho.

In all cases, the non-alignment can be obtained with an empty line. For class field, I feel very strongly that the code in much more readable for consecutive data member (and the issue of type names that are too long can be handle either with alias or with an empty line).

It's also a bigger departure from how code has recently been layed out

And this change 'revert' this inadvertent departure from the coding guidelines :)

[silverweed]

In all cases, the non-alignment can be obtained with an empty line

This is not a good solution at least for cxx files, as one is supposed to group variables for semantics rather than aesthetics. If you strongly want the class header variables to be aligned that's ok, but please let's find a way not to impact local variables and parameters.

[pcanal]

as one is supposed to group variables for semantics rather than aesthetics

Fair enough. Currently clang-format does not allow the distinction and as you mentioned aesthetics is less important in the cxx than the proper grouping, so it sounds like a decent compromise to turn it on for the sake of the header files. So let's merge this but not yet make it compulsory (i.e. unchanged stance to a failed clang-format check). Thanks.

[hahnjo]

Currently clang-format does not allow the distinction and as you mentioned aesthetics is less important in the cxx than the proper grouping, so it sounds like a decent compromise to turn it on for the sake of the header files. So let's merge this but not yet make it compulsory (i.e. unchanged stance to a failed clang-format check). Thanks.

This does not cut it because I (we?) use clang-format locally to format ntuple and histv7 development. The change to .clang-format leads to horrible formatting, here an example from the first file I tried:

diff --git a/hist/histv7/inc/ROOT/RAxes.hxx b/hist/histv7/inc/ROOT/RAxes.hxx
index 19848da1be1..3d95eba518b 100644
--- a/hist/histv7/inc/ROOT/RAxes.hxx
+++ b/hist/histv7/inc/ROOT/RAxes.hxx
@@ -53,7 +53,7 @@ public:
       }
    }

-   std::size_t GetNDimensions() const { return fAxes.size(); }
+   std::size_t                      GetNDimensions() const { return fAxes.size(); }
    const std::vector<RAxisVariant> &Get() const { return fAxes; }

    friend bool operator==(const RAxes &lhs, const RAxes &rhs) { return lhs.fAxes == rhs.fAxes; }
@@ -77,27 +77,27 @@ private:
    template <std::size_t I, std::size_t N, typename... A>
    RLinearizedIndex ComputeGlobalIndexImpl(std::size_t index, const std::tuple<A...> &args) const
    {
-      using ArgumentType = std::tuple_element_t<I, std::tuple<A...>>;
-      const auto &axis = fAxes[I];
+      using ArgumentType    = std::tuple_element_t<I, std::tuple<A...>>;
+      const auto      &axis = fAxes[I];
       RLinearizedIndex linIndex;
       if (auto *regular = axis.GetRegularAxis()) {
          if constexpr (std::is_convertible_v<ArgumentType, RRegularAxis::ArgumentType>) {

I revert the commit in #21629. Furthermore I argue the PR should not have been merged since there were pending rejections (by two team members!) that you did not wait if satisfied.

[pcanal]

Furthermore I argue the PR should not have been merged since there were pending rejections

My apologies. At the same time there was a couple of approval.

The change to .clang-format leads to horrible formatting,

I would argue that the 'current' clang-format leads to equally "horrible" formatting (in other places) but that the proposed layout as work-around its problem while the current/old layout as no work-around (beside clang-format off which 'works' as a work around as well or as bad in both cases). This asymmetry is why I proposed to merge.

For example the layout of the GetNDimensions is reverted to the desired one by introducing an empty line or a doc string:

   std::size_t GetNDimensions() const { return fAxes.size(); }

   const std::vector<RAxisVariant> &Get() const { return fAxes; }

or

   std::size_t GetNDimensions() const { return fAxes.size(); }
   ///< Some doc string
   const std::vector<RAxisVariant> &Get() const { return fAxes; }

The second ones can also be addresses with empty lines or with substituting assignment with initialization:

   using ArgumentType = std::tuple_element_t<I, std::tuple<A...>>;
   const auto      &axis{fAxes[I]};
   RLinearizedIndex linIndex;
   if (auto *regular = axis.GetRegularAxis()) {

(Side note, some might still not like the variable names alignment in this last case while I do personally find it more readable) but I of course recognize that this is a matter of taste).

In the end we have a situation where both choices have strong proponents/opponents (this PR was approved by some, its reversal was approved by some) and are incompatible. How can we move forward?

[silverweed]

How can we move forward?

A possible solution would be to introduce multiple .clang-format files (one per module), in order to preserve the different style choices of each. Of course this entails either deciding which style to adopt for every single module or to agree on a "default" style, which might just move the problem by just a few steps...

[pcanal]

A possible solution would be to introduce multiple .clang-format files (one per module), in order to preserve the different style choices of each ...

This seems technically workable (as in clang-format natively support this kind of setup and we are already using this for cling and llvm). Hopefully rather than having one per module, to only have one per module that deviate from the default (side note the sub-directory .clang-format can be limited to the 'deviation' from the default instead of being a full repeat).

[pcanal]

See follow-up/compromise PR at #21637