[WIP] Apply clang-format to WarpX.H, WarpX.cpp #5739
Conversation
EZoni
force-pushed
the
clangformat_continue
branch
from
b8ae4d6 to
dedbed9
Compare
There was a problem hiding this comment.
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.
| #ifdef WARPX_DIM_RZ | ||
| #include "BoundaryConditions/PML_RZ_fwd.H" | ||
| #include "FieldSolver/SpectralSolver/SpectralSolverRZ_fwd.H" | ||
| #else | ||
| #include "FieldSolver/SpectralSolver/SpectralSolver_fwd.H" | ||
| #endif |
There was a problem hiding this comment.
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.
| : public amrex::AmrCore | ||
| { | ||
| public: | ||
| class WARPX_EXPORT WarpX : public amrex::AmrCore { |
There was a problem hiding this comment.
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...
|
|
||
| static std::string Version (); //!< Version of WarpX executable | ||
| static std::string Version (); //!< Version of WarpX executable |
There was a problem hiding this comment.
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.
| amrex::Real a_full_dt, | ||
| int a_nl_iter, | ||
| bool a_from_jacobian ); | ||
| void ImplicitPreRHSOp (amrex::Real cur_time, amrex::Real a_full_dt, |
There was a problem hiding this comment.
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.
| @@ -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); | |||
There was a problem hiding this comment.
I am not completely sure about this change. I think that the original version was slightly more readable thanks to the extra spaces.
| * \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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Like:
\param[in] patch_type PatchType on which the field is initialized (fine
or coarse)
| } | ||
| namespace { | ||
|
|
||
| [[nodiscard]] bool |
There was a problem hiding this comment.
I know that it is not recommended, but I actually like the indentation inside a namespace. We may want to discuss this.
| 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, |
There was a problem hiding this comment.
Here I think that the original version was more readable. The main issue, however, is probably having methods with more than ~10 parameters!
Following up on #5687, this PR applies the current
clang-formatstyle, as is, to two more files, WarpX.H and WarpX.cpp. The code changes were implemented automatically by runningpre-commit run -afrom the root directory.This should be enough to showcase most of the style changes that this
clang-formatconfiguration (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.