[WIP] Apply clang-format to WarpX.H, WarpX.cpp #5739

EZoni · 2025-03-07T00:52:21Z

Following up on #5687, this PR applies the current clang-format style, as is, to two more files, WarpX.H and WarpX.cpp. The code changes were implemented automatically by running pre-commit run -a from the root directory.

This should be enough to showcase most of the style changes that this clang-format configuration (LLVM's default, with few minor adjustments introduced in #5687) produces.

The idea would be for reviewers to briefly go through the changes and raise objections, if any, which we could try to mitigate by adjusting the clang-format configuration further.

Of course it's still possible to object the whole idea of automatic style formatting, too.

lucafedeli88

Thanks a lot, @EZoni, for working on this!

I generally like the changes.

My main doubts concern:

indentation with pre-processor directives: I think that they are useful for readability!
opening curly brace on a new line for classes and functions : I personally like this style since it marks a difference with respect to curly braces after if(), while()...
automatic new lines for variables in function declarations: I think that sometimes we use newlines in function declaration to express ideas (e.g., some variables are related) and to improve readability. I am not sure that the proposed style changes are more readable than the original version.

See more detailed comments below.

lucafedeli88 · 2025-03-10T09:54:40Z

Source/WarpX.H

+#ifdef WARPX_DIM_RZ
+#include "BoundaryConditions/PML_RZ_fwd.H"
+#include "FieldSolver/SpectralSolver/SpectralSolverRZ_fwd.H"
+#else
+#include "FieldSolver/SpectralSolver/SpectralSolver_fwd.H"
+#endif


I am not completely sure about this style change. It seems to me that indentation was quite useful to make the code more readable here.

I would agree - the indentation makes it more readable.

This might be controlled by the following option:

So you both would prefer BeforeHash, correct?

Yes, BeforeHash.

lucafedeli88 · 2025-03-10T09:58:45Z

Source/WarpX.H

-    : public amrex::AmrCore
-{
-public:
+class WARPX_EXPORT WarpX : public amrex::AmrCore {


I am not completely sure about this style change.
I personally prefer the style:

void function_name () { } class MyClass { };

for functions and classes, and

if (/*..*/) { // ... }

for if, while, for...

I prefer the original. Putting it all on one line makes it harder to read. With the original, I can just glance at the code and see what it is doing. With it all on one line, I have to stare at it a moment to parse it out.

lucafedeli88 · 2025-03-10T09:59:58Z

Source/WarpX.H


-    static std::string Version (); //!< Version of WarpX executable
+    static std::string Version ();       //!< Version of WarpX executable


Sometimes we use spaces to align comments and make them more readable. I don't have a strong opinion on this specific change, but we may want to discuss it.

lucafedeli88 · 2025-03-10T10:04:05Z

Source/WarpX.H

-                            amrex::Real a_full_dt,
-                            int         a_nl_iter,
-                            bool        a_from_jacobian );
+    void ImplicitPreRHSOp (amrex::Real cur_time, amrex::Real a_full_dt,


Sometimes we use newlines in function declarations strategically to improve readability, e.g. :

void my_function ( double Ex, double Ey, double Ez, double Bx, double By, double Bz, int i, int j, int k, bool flag);

I think that we may want to discuss if and how we would like clang-format to change this for us.

I would agree here, that the layout of the arguments should be up to the developer to allow nice grouping.

See #5739 (comment).

Can the formatting of the routine arguments be turned off so that there is no standard? In this case, considering the wide variety of argument lists, there is no single standard that works well in all cases.

lucafedeli88 · 2025-03-10T10:06:46Z

Source/WarpX.H

@@ -489,22 +577,28 @@ public:
    void UpdateInjectionPosition (amrex::Real dt);

    void ResetProbDomain (const amrex::RealBox& rb);
-    void EvolveE (         amrex::Real dt, amrex::Real start_time);
+    void EvolveE (amrex::Real dt, amrex::Real start_time);


I am not completely sure about this change. I think that the original version was slightly more readable thanks to the extra spaces.

This is probably conflicting with #5739 (comment). Meaning, if we like one and don't like the other, or viceversa, there might be no way to make this systematic.

lucafedeli88 · 2025-03-10T10:09:25Z

Source/WarpX.H

-     * \param[in] patch_type PatchType on which the field is initialized (fine or coarse)
-     * \param[in] eb_update_field flag indicating which gridpoints should be modified by this functions
-     * \param[in] use_eb_flags (default:true) flag indicating if eb points should be excluded or not
+     * \param[in] patch_type PatchType on which the field is initialized (fine


While I like the idea of stetting a limit for the number of columns used for docstrings, I think that we may need an extra indentation here for the description of the input parameters.

Like:

\param[in] patch_type PatchType on which the field is initialized (fine or coarse)

I am not sure if it is possible to customize this in a way that is systematic, beyond wrapping the relevant lines in a "don't format" wrap.

lucafedeli88 · 2025-03-10T10:46:03Z

Source/WarpX.cpp

-    }
+namespace {
+
+[[nodiscard]] bool


I know that it is not recommended, but I actually like the indentation inside a namespace. We may want to discuss this.

I would agree that it is better to use indentation inside a name space. This makes it readily clear that the routines defined are in a defined space.

I think it should be possible to guarantee this by replacing LLVM's

NamespaceIndentation: None

with

NamespaceIndentation: All

lucafedeli88 · 2025-03-10T10:51:09Z

Source/WarpX.cpp

-        m_safe_guard_cells,
-        WarpX::do_multi_J,
-        WarpX::fft_do_time_averaging,
+        dt[lev], dx, m_do_subcycling, WarpX::use_fdtd_nci_corr, grid_type,


Here I think that the original version was more readable. The main issue, however, is probably having methods with more than ~10 parameters!

Here too I am not sure if there is a way to make this systematic, as one version might look more readable in one place, the other version in another place. I personally like the Ruff linting we use in Python, which enforces one argument per line, except for very short lines, because it makes reviewing code changes easier.

These might be our only options:

dpgrote · 2025-03-11T18:52:52Z

Source/WarpX.H

-{
-public:
+class WARPX_EXPORT WarpX : public amrex::AmrCore {
+  public:


Maybe it's just what I'm used to, but I prefer no indentation on the public, 'private', `protected' lines.

This might be controlled by IndentAccessModifiers?

dpgrote · 2025-03-11T18:55:15Z

Source/WarpX.H


    /** Destructor */
    ~WarpX () override;

    /** Copy constructor */
-    WarpX ( WarpX const &) = delete;
+    WarpX (WarpX const&) = delete;


I approve of this standard. I've never liked the extra spaces before the arguments (and at the end of the list).

dpgrote · 2025-03-11T19:01:57Z

Source/WarpX.H

-    amrex::Vector<std::array< std::unique_ptr<amrex::iMultiFab>,3 > >& GetEBUpdateEFlag() { return m_eb_update_E; }
-    amrex::Vector<std::array< std::unique_ptr<amrex::iMultiFab>,3 > >& GetEBUpdateBFlag() { return m_eb_update_B; }
-    amrex::Vector< std::unique_ptr<amrex::iMultiFab> > const & GetEBReduceParticleShapeFlag() const { return m_eb_reduce_particle_shape; }
+    void SetElectricFieldAndApplyBCs (const WarpXSolverVec& a_E,


Presumably these changes are being made because of the line length limit. I've always preferred longer lines which, to me, makes this type of code easier to read and more compact. What is the maximum line length?

The LLVM configuration file sets

ColumnLimit: 80

so it's probably 80 at the moment.

EZoni · 2025-03-12T00:09:40Z

@lucafedeli88 @dpgrote

I will try to go through each of your comments and see if there is a corresponding option that can be set to match your suggestions.

However, I'm starting to think that we might need to choose a strategy to move forward.

Should we discuss in the Technical Committee every formatting change that receives an objection? I am afraid that this might not be interesting for some. On the other hand, I or others might not agree with your or others' preferences, in which case a vote would be the only way to resolve, so I'm not sure.

EZoni added the cleaning Clean code, improve readability label Mar 7, 2025

Apply clang-format to WarpX.H, WarpX.cpp

dedbed9

EZoni force-pushed the clangformat_continue branch from b8ae4d6 to dedbed9 Compare March 7, 2025 00:58

lucafedeli88 reviewed Mar 10, 2025

View reviewed changes

dpgrote reviewed Mar 11, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Apply clang-format to WarpX.H, WarpX.cpp #5739

[WIP] Apply clang-format to WarpX.H, WarpX.cpp #5739

EZoni commented Mar 7, 2025 •

edited

Loading

lucafedeli88 left a comment •

edited

Loading

lucafedeli88 Mar 10, 2025

dpgrote Mar 11, 2025

EZoni Mar 12, 2025 •

edited

Loading

dpgrote Mar 12, 2025

lucafedeli88 Mar 10, 2025

dpgrote Mar 11, 2025

lucafedeli88 Mar 10, 2025

lucafedeli88 Mar 10, 2025

dpgrote Mar 11, 2025

EZoni Mar 12, 2025

dpgrote Mar 12, 2025

lucafedeli88 Mar 10, 2025

EZoni Mar 12, 2025

lucafedeli88 Mar 10, 2025

lucafedeli88 Mar 10, 2025

EZoni Mar 12, 2025 •

edited

Loading

lucafedeli88 Mar 10, 2025

dpgrote Mar 11, 2025

EZoni Mar 12, 2025 •

edited

Loading

lucafedeli88 Mar 10, 2025

EZoni Mar 12, 2025

dpgrote Mar 11, 2025

EZoni Mar 12, 2025

dpgrote Mar 11, 2025

dpgrote Mar 11, 2025

EZoni Mar 12, 2025

EZoni commented Mar 12, 2025


		static std::string Version (); //!< Version of WarpX executable
		static std::string Version (); //!< Version of WarpX executable

[WIP] Apply clang-format to WarpX.H, WarpX.cpp #5739

Are you sure you want to change the base?

[WIP] Apply clang-format to WarpX.H, WarpX.cpp #5739

Conversation

EZoni commented Mar 7, 2025 • edited Loading

lucafedeli88 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EZoni Mar 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EZoni Mar 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EZoni Mar 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EZoni commented Mar 12, 2025

EZoni commented Mar 7, 2025 •

edited

Loading

lucafedeli88 left a comment •

edited

Loading

EZoni Mar 12, 2025 •

edited

Loading

EZoni Mar 12, 2025 •

edited

Loading

EZoni Mar 12, 2025 •

edited

Loading