Fix: apply clang-tidy fixes #4015

caic99 · 2024-04-18T04:40:17Z

Reminder

Have you linked an issue with this pull request?
Have you added adequate unit tests and/or case tests for your pull request?
Have you noticed possible changes of behavior below or in the linked issue?
Have you explained the changes of codes in core modules of ESolver, HSolver, ElecState, Hamilt, Operator or Psi? (ignore if not applicable)

Linked Issue

Fix #3915

Unit Tests and/or Case Tests for my changes

A unit test which is expected to exit with assertion error has been fixed.

What's changed?

This PR applies clang-tidy rules on existing codes to improve code quality. I've carefully reviewed the modifications.

haozhihan · 2024-04-19T02:55:17Z

According to Commit message guidelines, I think using keyword Style might be more appropriate.

caic99 · 2024-04-19T04:04:23Z

According to Commit message guidelines, I think using keyword Style might be more appropriate.

This PR makes actual changes to the code.

source/module_base/intarray.h

mohanchen

I have looked through dozens of files, most of them look good to me. Howver, I don't understand some modifications, as can be seen with my comments.

source/module_base/abfs-vector3_order.cpp

mohanchen · 2024-04-21T08:37:41Z

source/module_base/complexarray.h

@@ -117,7 +117,7 @@ void scaled_sum(std::complex <double> c1, const ComplexArray &cd1,
           ComplexArray &cd3);

 /// out[i] = a1[i] * in2[i]
-void point_mult(ComplexArray &a1, ComplexArray &in2, ComplexArray &out);
+void point_mult(ComplexArray &in1, ComplexArray &in2, ComplexArray &out);


I feel the meaning has been changed here

The name is changed in consistent with that in complexarray.cpp.

mohanchen · 2024-04-21T08:40:14Z

source/module_base/inverse_matrix.h

+	double *e{};
+	int lwork{};
+	std::complex<double> *work2{};
+	double* rwork{};


why not use nullptr here?

Using {} is identical, as it initializes the pointer to nullptr.

I am not looking identical ways of writing C++ codes, I just consider this new way of initializing variables make reading more difficult for some people, we should have a unified way. For example, double *rwork = nullptr;

mohanchen · 2024-04-21T08:40:59Z

source/module_base/kernels/math_op.cpp

 namespace ModuleBase {

 template <typename FPTYPE>
 __inline__
-FPTYPE __fact(const int n) {
+FPTYPE _fact(const int n) {


is there any special reason to change the function name from __ to _

The C++ standard reserves names starting with __.

mohanchen · 2024-04-21T08:41:27Z

source/module_base/math_chebyshev.h

@@ -187,7 +187,7 @@ class Chebyshev

 public:
 	//Members:
-    int norder;   // order of Chebyshev expansion
+    int norder{};   // order of Chebyshev expansion


why not initialize it will 0?

It will, please see https://en.cppreference.com/w/cpp/language/zero_initialization

I mean, why not write the code "int norder = 0" ?

I mean, why not write the code "int norder = 0" ?

Using bracelet is the only supported grammar for this fix initializing members in declaration. There is another fix available in your expected grammar, but the modifications will be much more.

source/module_base/math_sphbes.cpp

source/module_base/module_container/ATen/ops/linalg_op.h

mohanchen · 2024-04-21T08:43:50Z

source/module_base/opt_DCsrch.cpp

-                                        bool&,
-                                        double&,
-                                        double&);
+    extern /* Subroutine */ void dcstep(double& /*stx*/,


looks strange here

Add variable names according to declaration to improve code readability.

We don't recommend writing "/* */"

source/module_base/parallel_global.cpp

source/module_base/spherical_bessel_transformer.cpp

jinzx10 · 2024-04-21T16:45:03Z

I went through the files in module_base, module_basis and module_cell. Apart from the mistaken removal of some gpu code in pw_basis_sup.cpp (which has been fixed in a commit), clang-tidy indeed tidies up a lot of code. Here's a brief list of what I record:

add const& to function argument that are not altered in the body
This can save many uncessary copy and is definitely a good practice.
use std::move when copying variables that are no longer needed
This can also prevent unnecessary copy (given that the move constructor / move assignment operator is available)
delete "return" for void functions
delete "void" in function parameter list
These are basically a matter of style. Maybe we should choose one and stick with it. (PS: In C, a function declared as func() can take any arguments, while a function that takes no argument should be declared as func(void). In C++, func() is equivalent to func(void) so in some sense func(void) is more compatible and func() is more C++)
reserve a std::vector before looping over push_back
This is definitely a good practice if one can somehow know or estimate the final size in advance. If the objects are expensive to copy, or if there're a lots of elements to push back, reserving a vector could save a lot of time.
replacing NULL or 0 with nullptr
good practice by c++ standard
more concise code when using bool variables
If x is a bool, if statement can simply be if(x) instead of if (x == true) (or if(!x) instead of if (x == false)). I believe these two styles have no essential difference as the compiler would be smart enough to generate the same code, but I personally do prefer the shorter one. Another case is

if (some_condition)
    x = true;
else
    x = false;

which can be simply

x = some_condition;

use the member function empty() when checking if a stl container is empty
Technically empty() provided by the container is the fastest way to check if a container is empty. Take vector as an example, x.size() == 0 actually involves two steps: (1) subtracting "begin" from "finish" and (2) compare the result with 0, while empty() simply performs one step "begin == finish". Though the difference is marginal and might be optimized by the compiler, I think it's always better to stick with the better one.
make member functions static whenever possible
This is important for unit test. Static member function can be called (thus tested) without constructing object.
initialize POD type variables (int, double, pointer, ...)
This is very important to avoid many unexpected behaviors. An uninitialized int/double/pointer could be any random value. What's even worse is that the piece of memory allocated to those uninitialized variables are used before and is occupied by some meaingful value. In that case, even if the code has some logical error, such uninitialized variables might accidentally make things work. There's actually a fresh example in this PR: in source/module_cell/test/read_pp_test.cpp, HeaderErr2015, an extra "EXPECT_DEATH" has to be added to the tested function which is supposed to fail. The reason that the function could normally terminate before this PR is that the test involves an assertion on an uninitialized variable which was set in the last unit test HeaderErr2013. Even though test objects are intialized for each test in unit testing, uninitialized variables could actually "inherit" values from previous tests if memory are allocated the same way.
no need to initialize a string with "" (and other stl containers to explicit empty)
As opposed to POD types, default intializations of stl containers are all properly handled - e.g., std::string is default initialized to "" - so there's no need to specify.
when finding a single character in a string, make sure to pass a char instead of a string with one character:

str.find(':'); // good
str.find(":"); // not as good, though practically the difference might be neglible

in an if-else statement, removing the "else-branch" if the "if-branch" contains something that terminates the code flow:
for example, clang-tidy constantly replaces

if (condition)
    return some_variable;
else
    do_something_else();

with

if (condition)
    return some_variable;
do_something_else();

I think this is another matter of style which I have no particular preference. However, if both branches return something, I personally prefer the ternary operator:

return (condition) ? A : B;

For the three modules I was assigned, I think the changes made by this PR looks good (with some style issue that may or may not need discussion). The only real issue is that this PR messes up quite a few indentation, which might be fixed by another PR of clang-format, I guess?

caic99 · 2024-04-22T02:45:38Z

The only real issue is that this PR messes up quite a few indentation, which might be fixed by another PR of clang-format, I guess?

Running clang-format will modify codes significantly, so this PR does not execute it.

kirk0830

I have reviewed files in module_hamilt_pw, module_io, module_psi.
There are mainly several kinds of code polishment I have observed:

change "pass by value" to "pass by ref" or "pass by value then swap".
add static keyword ahead of function that does not rely on member variables.
delete useless return in void function.
change some 0 and 1 to false or true, correspondingly, avoid implicit type conversion
use list initialization on PART of member variables.
remove the use of operator ?: in some boolean assignment cases.
add annotation behind namespace.
make nomenclature of varialbles consistent.
delete redundant if not nullptr sentence before deallocating one pointer
change the default initialization value NULL for pointer to nullptr

However,

it (clang-tidy) indeed removed some annotation by mistake especially at the end of functions.
the indent seems to be not well-treated, but I guess it is because the mixed use of Tab and whitespaces
not fully change the if-else logic, like in psi_initializer_atomic.cpp:

std::complex<double> phase_factor(double arg, int mode)
{
    if(mode == 1) return std::complex<double>(cos(arg),0);
    if (mode == -1) return std::complex<double>(0, sin(arg));
    else if (mode == 0) return std::complex<double>(cos(arg), sin(arg));
// can also be `if` instead of `else if`
    else return std::complex<double>(1,0);
// can directly `return`
}

source/module_io/binstream.cpp

source/module_io/cal_dos.cpp

source/module_io/cal_dos.h

source/module_io/istate_envelope.h

kirk0830 · 2024-04-22T01:51:38Z

source/module_hamilt_pw/hamilt_pwdft/operator_pw/meta_pw.h

@@ -26,7 +26,7 @@ class Meta<OperatorPW<T, Device>> : public OperatorPW<T, Device>
  private:
    using Real = typename GetTypeReal<T>::type;
    public:
-      Meta(Real tpiba2_in,
+      Meta(Real tpiba_in,


I am not sure but it seems tpiba is not equivalent with tpiba2, the latter seems to be square of the former.

kirk0830 · 2024-04-22T01:52:30Z

source/module_hamilt_pw/hamilt_pwdft/operator_pw/velocity_pw.h

@@ -35,8 +35,8 @@ class Velocity
     */
    void act(const psi::Psi<std::complex<double>>* psi_in,
             const int n_npwx,
-             const std::complex<double>* tmpsi_in,
-             std::complex<double>* tmhpsi,
+             const std::complex<double>* psi0,


this seems not a good idea...

source/module_hamilt_pw/hamilt_pwdft/stress_func_cc.cpp

caic99 · 2024-04-24T09:34:37Z

Hi @mohanchen @kirk0830 ,
I've replied the conversations and modified codes w.r.t. our discussion. Would you please kindly review the PR? Feel free to comment if you have further questions. Many thanks.

mohanchen · 2024-04-27T13:18:03Z

Hi @mohanchen @kirk0830 , I've replied the conversations and modified codes w.r.t. our discussion. Would you please kindly review the PR? Feel free to comment if you have further questions. Many thanks.

Hi Cun, though most of the updates look good to me. However, I am deeply concerned about some modifications that change the parameter names, and the amount of modifications of codes is huge (522 files!). In addition, although I ask some questions, I don't get the answers I want (not the explanations for C++ grammar).

caic99 · 2024-04-28T03:58:51Z

Hi Cun, though most of the updates look good to me. However, I am deeply concerned about some modifications that change the parameter names, and the amount of modifications of codes is huge (522 files!). In addition, although I ask some questions, I don't get the answers I want (not the explanations for C++ grammar).

Hi @mohanchen ,
I fully understand your concern and am agree with you.
This PR acts as an experiment to provide a possible solution for using automatic tools to improve code quality, and actually I'm not expecting it will be merged into trunk. However, I think the most important point here is that providing us a chance to discuss the rules in detail to maintain code quality.

mohanchen · 2024-05-08T13:58:30Z

Hi Cun, though most of the updates look good to me. However, I am deeply concerned about some modifications that change the parameter names, and the amount of modifications of codes is huge (522 files!). In addition, although I ask some questions, I don't get the answers I want (not the explanations for C++ grammar).

Hi @mohanchen , I fully understand your concern and am agree with you. This PR acts as an experiment to provide a possible solution for using automatic tools to improve code quality, and actually I'm not expecting it will be merged into trunk. However, I think the most important point here is that providing us a chance to discuss the rules in detail to maintain code quality.

Thanks, I understand your thoughts better now. How about we close this PR but we label it as 'Useful Information', then we can use this as a reference for further discussion of how to make changes as we expect. @caic99

caic99 · 2024-05-27T03:35:24Z

Hi all,
I've updated the clang-tidy rules discussed above.

in an if-else statement, removing the "else-branch" if the "if-branch" contains something that terminates the code flow:
for example, clang-tidy constantly replaces

I've removed this rule removing else after a return statement. This is indeed optional, and will making incomplete changes.

the indent seems to be not well-treated

This is the job of clang-format, and we may discuss it with another issue.

However, I still insist that some fixes are necessary, which we've also discussed above (and I kept them unresolved to show more details):

Consistency between declaration and definition
Zero-initialize variables

By consolidating a rule and applying them on each PR, the code quality is able to be assured and improved during refactor.

deepmodeling#4015

* CI: add pre-commit.ci to apply clang-format and clang-tidy fixes * do pre-commit fixes first * install clang utils * apply fixes only on changed files * fetch historical commits to get the changed files * test if pre-commit could fix sabotaged format * update pre-commit config * apply fixes * commit changes after apply them * commit changes using pre-commit.ci lite * try to fix SSL error using pre-commit.ci lite * [pre-commit.ci lite] apply automatic fixes * format arguments * add docs on pre-commit hook * Update clang-format config. ABACUS uses some functions with a long function name and a long parameter list. Current setup will generates ugly parameter list with each parameters indented by many spaces. Plus, it is unable to use a customized layout for e.g. putting x, y, and z value in the same line. After discussion, we agree to disable those settings, leaving developers free to chose their own flavor. * Update clang-tidy config. #4015 * [pre-commit.ci lite] apply automatic fixes * Update .pre-commit-config.yaml * Update .clang-tidy according to the meeting --------- Co-authored-by: pre-commit-ci-lite[bot] <117423508+pre-commit-ci-lite[bot]@users.noreply.github.com> Co-authored-by: Mohan Chen <mohan.chen.chen.mohan@gmail.com> Co-authored-by: Haozhi Han <haozhi.han@outlook.com>

caic99 added 2 commits April 18, 2024 12:02

update clang-tidy config for fixing

7aa9ee1

apply run-clang-tidy-12 -fix and adjust some false cases

15364d8

caic99 marked this pull request as draft April 18, 2024 04:40

caic99 added 7 commits April 18, 2024 12:52

adjust UT

bfd8aeb

fix unitcell ut

d198f83

fix read atoms

75fc803

patch paw

82a3555

fix paw

775f495

fix paw init

f5c4ab1

fix read pp UT

e11c234

caic99 marked this pull request as ready for review April 18, 2024 08:20

caic99 added the Code Quality label Apr 18, 2024

caic99 requested a review from WHUweiqingzhou April 18, 2024 08:20

WHUweiqingzhou requested review from kirk0830 and jinzx10 April 19, 2024 07:04

jinzx10 reviewed Apr 19, 2024

View reviewed changes

source/module_base/intarray.h Show resolved Hide resolved

fix gpu branch

e1c7d74

mohanchen reviewed Apr 21, 2024

View reviewed changes

Merge branch 'develop' into clang-tidy-fix

007f77d

caic99 added 2 commits April 22, 2024 10:52

fix tests in parallel_global.cpp

560ca30

not applying readability-else-after-return

8190907

caic99 requested a review from mohanchen April 22, 2024 03:00

kirk0830 reviewed Apr 22, 2024

View reviewed changes

caic99 and others added 3 commits April 22, 2024 12:20

Update source/module_io/binstream.cpp

8464591

Update source/module_io/cal_dos.cpp

4cbf8ec

Merge branch 'develop' into clang-tidy-fix

a2120a9

caic99 requested a review from kirk0830 April 24, 2024 09:35

mohanchen added Plans and removed Code Quality labels May 5, 2024

mohanchen added Useful Information Useful information for others to learn/study and removed Plans labels May 8, 2024

caic99 closed this May 8, 2024

caic99 added a commit to caic99/abacus-develop that referenced this pull request May 27, 2024

Update clang-tidy config.

3eeee12

deepmodeling#4015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: apply clang-tidy fixes #4015

Fix: apply clang-tidy fixes #4015

caic99 commented Apr 18, 2024 •

edited

Loading

haozhihan commented Apr 19, 2024

caic99 commented Apr 19, 2024

mohanchen left a comment

mohanchen Apr 21, 2024

caic99 Apr 22, 2024

mohanchen Apr 21, 2024

caic99 Apr 22, 2024

mohanchen Apr 27, 2024

mohanchen Apr 21, 2024

caic99 Apr 22, 2024

mohanchen Apr 21, 2024

caic99 Apr 22, 2024

mohanchen Apr 27, 2024

caic99 Apr 28, 2024

mohanchen Apr 21, 2024

caic99 Apr 22, 2024

mohanchen Apr 27, 2024

jinzx10 commented Apr 21, 2024

caic99 commented Apr 22, 2024

kirk0830 left a comment

kirk0830 Apr 22, 2024

mohanchen Apr 27, 2024

kirk0830 Apr 22, 2024

caic99 commented Apr 24, 2024 •

edited

Loading

mohanchen commented Apr 27, 2024

caic99 commented Apr 28, 2024

mohanchen commented May 8, 2024

caic99 commented May 27, 2024

Fix: apply clang-tidy fixes #4015

Fix: apply clang-tidy fixes #4015

Conversation

caic99 commented Apr 18, 2024 • edited Loading

Reminder

Linked Issue

Unit Tests and/or Case Tests for my changes

What's changed?

haozhihan commented Apr 19, 2024

caic99 commented Apr 19, 2024

mohanchen left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jinzx10 commented Apr 21, 2024

caic99 commented Apr 22, 2024

kirk0830 left a 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

caic99 commented Apr 24, 2024 • edited Loading

mohanchen commented Apr 27, 2024

caic99 commented Apr 28, 2024

mohanchen commented May 8, 2024

caic99 commented May 27, 2024

caic99 commented Apr 18, 2024 •

edited

Loading

caic99 commented Apr 24, 2024 •

edited

Loading