Retrying clang-format

[cmdoug]

No real changes to src or etc folders, just ran the lines:

git ls-files 'src/*.cpp' 'src/*.hpp' 'src/*.c' 'src/*.h' | xargs -n1 clang-format -i
git ls-files 'etc/*.cpp' 'etc/*.hpp' 'etc/*.c' 'etc/*.h' | xargs -n1 clang-format -i

In the plugin folder, some /* clang-format off */ and /* clang-format on */ were added, then ran:

git ls-files 'plugin/*.cpp' 'plugin/*.hpp' 'plugin/*.c' 'plugin/*.h' | xargs -n1 clang-format -i

Ref: f715746

[cmdoug]

Note that I did essentially nothing here. From what I can tell, all credit is to @sgarnotel. Some questions for review:

• Should .clang-format also be enforced via GitHub Actions or is that overkill?
• This will wreck git blame. Do we want to avoid that? E.g. via --ignore-rev
• Also, to avoid further commits with huge diffs, might be worth asking: is the existing .clang-format exactly what we want?

[prj-]

1. There is currently no strict rule for merging, so there is not much to enforce.
2. There is no way around that, but anyway, the history is already messed up since around the release of version 4.
3. I guess the one which minimizes changes would be ideal.

[cmdoug]

Sounds good. In response to 3., do you (or the other developers) have any suggestions for alterations to .clang-format that would minimize changes while still cleaning up formatting issues? I just used the one already in the project.

[prj-]

Sorry for the unbelievably late answer, but reusing the current .clang-format is fine. The CI is currently broken, I had hoped to get an answer from @simonlegrand by now, but until then, I'm very wary of merging such a large PR.i

[cmdoug]

No worries. I agree that patience is a virtue with such a large PR. Also, since there is no functional change, there is no urgency for getting this merged. Perhaps this could be wrapped in with the next major release.

[1995hnagamin]

I noticed that ColumnLimit: 200 might be too permissive and collapses multi-line statements that should remain split for readability.
Using ColumnLimit: 0 (no limit) would preserve the existing formatting without enforcing automatic breaks.

ColumnLimit 200

   // 2 <= NCV-NEV and NCV <= N
-  if (!(2 <= nbev && ncv <= n))
-    serr[err++] = "   ( 2 <= nbev && nvc <= n) ";
-  if (n != OP1.N)
-    serr[err++] = " the first matrix in EigneValue is not Hermitien.";
-  if (n != B.N)
-    serr[err++] = "Sorry the row's number of the secand matrix in EigneValue is wrong.";
-  if (n != B.M)
-    serr[err++] = "Sorry the column's number of the secand matrix in EigneValue is wrong.";
-  if (verbosity)
-    cout << "Complex eigenvalue problem: A*x - B*x*lambda" << endl;
-  if (verbosity > 9 || err)
-    cout << "   n " << n << " nev "<< nbev << " tol = " << tol << " maxit = " << maxit << " ncv = " << ncv
-      << " driver = " << driver << " which = " << which << endl;
+  if (!(2 <= nbev && ncv <= n)) serr[err++] = "   ( 2 <= nbev && nvc <= n) ";
+  if (n != OP1.N) serr[err++] = " the first matrix in EigneValue is not Hermitien.";
+  if (n != B.N) serr[err++] = "Sorry the row's number of the secand matrix in EigneValue is wrong.";
+  if (n != B.M) serr[err++] = "Sorry the column's number of the secand matrix in EigneValue is wrong.";
+  if (verbosity) cout << "Complex eigenvalue problem: A*x - B*x*lambda" << endl;
+  if (verbosity > 9 || err) cout << "   n " << n << " nev " << nbev << " tol = " << tol << " maxit = " << maxit << " ncv = " << ncv << " driver = " << driver << " which = " << which << endl;
   if (err) {

ColumnLimit 0

@@ -914,8 +925,8 @@ AnyType EigenValueC::E_EV::operator () (Stack stack) const {
   if (verbosity)
     cout << "Complex eigenvalue problem: A*x - B*x*lambda" << endl;
   if (verbosity > 9 || err)
-    cout << "   n " << n << " nev "<< nbev << " tol = " << tol << " maxit = " << maxit << " ncv = " << ncv
-      << " driver = " << driver << " which = " << which << endl;
+    cout << "   n " << n << " nev " << nbev << " tol = " << tol << " maxit = " << maxit << " ncv = " << ncv
+         << " driver = " << driver << " which = " << which << endl;
   if (err) {
     cerr << " list of the error " << endl;
     for (int i = 0; i < err; ++i)
@@ -923,7 +934,7 @@ AnyType EigenValueC::E_EV::operator () (Stack stack) const {

[cmdoug]

@1995hnagamin I see your point, but I can also see the counterpoint. (Note that ColumnLimit: 200 was in the .clang-format file even before this PR.) Would be nice to get input from others if any other readers have thoughts on this.

[cmdoug]

Converting this back to draft for now. Can deal with merge conflicts and specific formatting decisions at a later time.

[simonlegrand]

Sorry for this really late comment... To complete this PR, it could be nice to automate the process of formatting for the future. I can think of two common ways to do that:

• Pre-commit hooks. But it forces developers to modify there development environment. I'm personally not a fan with languages like c/c++ where there are a lot of different toolchains.
• A check in the CI pipeline. We could call a wokflow that would fail if formatting is not respected, something like

name: Format Check
on:
  workflow_call:
jobs:
  format:
    runs-on: ubuntu-latest
    steps:
    ...
      - name: Check formatting
        run: |
          sudo apt-get install clang-format
          clang-format --dry-run --Werror {src, etc, plugin}/**/*.{h, hpp, c,cpp}.

My few cents...

[prj-]

We just did this in HPDDM: hpddm/hpddm#137. Relying on pre-commit hooks is a no-go as you mentioned. Relying on clang-format from aptitude is also a no-go since it is lagging behind significantly (we are at version 21, but we would be stuck at 18).

[cmdoug]

Hi @simonlegrand, thank you for the input. No worries about the delay.

I agree that a CI check would be preferable to the alternative. In case of a formatting failure, one could also have the pipeline automatically run clang-format on the affected code and then automatically retry the pipeline, no? Just thinking of how to minimize silly failures due to imperfect formatting.

[simonlegrand]

@prj- Yes we should ensure a recent version of clang-format. On ubuntu 24.04, apt has version 20 available. Otherwise on a macos runner, we could have the last version with brew.
@cmdoug It would be nice to have such complete automation but I see what seems to me a major drawback: users would have to resync their local branch every time there is a formatting commit. Leaving the formatting task to the user might be annoying at first, but the logs generated by clang-format would greatly help the process.