ENH: add clang-format file

[ferdymercury]

This makes it much easier to contribute to the project, since IDEs auto-save files using the proper formatting. No one needs to know the 100 rules by heart, the IDE takes care of it and saves time of reviewing Pull Requests.

Format file copied from Slicer/Slicer#7603

[cpinter]

Thanks! Does this preserve the current style more or less?

[ferdymercury]

Not fully, but probably it can be adjusted if SlicerRT follows a different convention than Slicer. Specially the column limit. This is an example file:

index c8ed6d95..6476d795 100644
@@ -85,3 +85,8 @@ QString qSlicerRoomsEyeViewModule::helpText()const
-  return QString("This module loads and visualizes treatment machines, and calculates collisions between its parts and the patient."
-    "For more information see <a href=\"%1/Documentation/%2.%3/Modules/RoomsEyeView\">%1/Documentation/%2.%3/Modules/RoomsEyeView</a><br>").arg(
-    this->slicerWikiUrl()).arg(qSlicerCoreApplication::application()->majorVersion()).arg(qSlicerCoreApplication::application()->minorVersion());
+  return QString(
+           "This module loads and visualizes treatment machines, and calculates collisions between its parts and the "
+           "patient."
+           "For more information see <a "
+           "href=\"%1/Documentation/%2.%3/Modules/RoomsEyeView\">%1/Documentation/%2.%3/Modules/RoomsEyeView</a><br>")
+    .arg(this->slicerWikiUrl())
+    .arg(qSlicerCoreApplication::application()->majorVersion())
+    .arg(qSlicerCoreApplication::application()->minorVersion());
@@ -93 +98,4 @@ QString qSlicerRoomsEyeViewModule::acknowledgementText()const
-  return "This work is part of SparKit project, funded by Cancer Care Ontario (CCO)'s ACRU program and Ontario Consortium for Adaptive Interventions in Radiation Oncology (OCAIRO).\n\nThe collision detection module was partly supported by Conselleria de Educación, Investigación, Cultura y Deporte (Generalitat Valenciana), Spain under grant number CDEIGENT/2019/011.";
+  return "This work is part of SparKit project, funded by Cancer Care Ontario (CCO)'s ACRU program and Ontario "
+         "Consortium for Adaptive Interventions in Radiation Oncology (OCAIRO).\n\nThe collision detection module was "
+         "partly supported by Conselleria de Educación, Investigación, Cultura y Deporte (Generalitat Valenciana), "
+         "Spain under grant number CDEIGENT/2019/011.";

[cpinter]

The length limit seems to hurt readability, at least for me. Three lines became eight, with one line containing only one word. I'd strongly prefer not introducing such line breaks in the code...

[lassoan]

When a developer writes the code then he tries to make the code easy to read based on his understanding of what the code does (e.g., newlines and indentations). Some other formattings just happen to be like that, not a result of conscious decision (e.g., whitespace around operators). Unfortunately, clang cannot distinguish between the two, it just aggressively enforces rules based on semantics and ignoring meaning and most of the original formatting.

We could insert special hints into the code that (comments, commas, or turn off/on clang formatting locally) to make sure intentional formatting choices are preserved. But we should not have to litter the code with this kind of fluff just to please a source code formatter.

I expect new formatting tools will be available that either can take more hints from the existing source code or make intelligent decisions based on the meaning of the code. They will be able to fix unnecessary inconsistencies in the code without removing all useful hints in the code from the original developers.

What specific next steps we can make? I think current clang version is hopeless. If you set a very large ColumnLimit then it will blindly merge multiple lines that are meant to be separate, if you set a low value then it will split up lines that we want to keep together. We could try to play with various weights to reduce the damage to the code, but I'm not sure if it is worth the time. Instead, we could try some modern formatters that use LLMs to understand the code and make formatting decisions based on that.

@ferdymercury would you have time to explore how well modern formatters work?

[ferdymercury]

Hi, thanks a lot @lassoan for the feedback! It sounds interesting, however I do not have time at the moment for exploring into this direction of LLM.

Also, the beauty of clang-format is that git-clang-format exists. And that is well integrated in IDEs.
And it should not modify old files. It will just apply to new code, and it does not need to be mandatory, one can revert something if disliked.

What has worked for me quite well for very large projects is to define a set of conservative rules and penalties that kind of work well 95% of the time, and then use

// clang-format off
// clang-format on

for blocks of code where one wants to actively disable that.

[ferdymercury]

Concerning line breaks, there is a wrapper script workaround described in this feature request: llvm/llvm-project#58417

[lassoan]

Considering current limitations of clang-format, I would rather not use it at all - as the number of formatting-related problems it creates is comparable to the number of problems it solves. However, I haven't contributed to this repository for years, so currently active developers should decide how they like to work.

[ferdymercury]

Thanks!
I partially agree: I think it's different for expert developers (you, Csaba) than for newcomers like me. For me it would help in a first round more than the problems it would create, since I do not know anything about the envisioned format. When you write, true, it's equally annoying than helpful. But that's because you already know it by heart how it should be, unlike a newcomer.

I daily contribute to ROOT / CERN 100k commits software. When I first started there I did not need to know or think what format they like, since they have a clangformat file and the CI workflow makes suggestions on what to change from your PR. Thats why I am biased towards suggesting one, since I am used to it working 85÷ well.

I am totally fine with closing this PR too, but in that case I would strongly suggest writing a didactic Markdown file explaining what the coding format convention should be :)

[lassoan]

writing a didactic Markdown file explaining what the coding format convention should be :)

Slicer extensions are based on Slicer, which is based on VTK, so we follow those conventions by default. Probably there is no need to spend time with developing and maintaining a style guide specifically for this extension.

As long as clang-format restrictions are not enforced on commit and developers manage whitespace changes reasonably (e.g., do not let in whitespace changes that decrease readability and don't mix whitespace changes with functional changes in a single commit), it should be all good. Having a .clang-format file to the source tree does not directly violate these requirements (developers just be a bit more careful), so I think it could be added, but it is up to you, @cpinter, and other active developers to decide.

[cpinter]

Thanks for the discussion. I agree with @ferdymercury that for newcomers it would be useful to treat formatting automatically, but applying the formatting to the whole codebase seems like a really bad idea. I don't know how hard would it be to introduce a system that automatically applies the formatting on PRs but only the actual changes, but doesn't touch the code. But in case of this particular project, our regular collaborators are quite diligent (and there are basically no one-off contributors), and I think making this change would take way more resources than the gain it would bring. Fernando, if you see a relatively simple way to use the formatter only on incoming changes, I'm open to it though.

[ferdymercury]

It's rather easy to restrict changes to the PR by calling git-clang-format.

We would just need 10 lines in a yml file

See https://github.com/root-project/root/pull/18801/files

Also, this does just adds suggestions to the PR, the user then decides whether applying them or not.