Skip to content

Commit c9bae03

Browse files
mohanchenabacus_fixer
andauthored
Add a python script to test the code quality of ABACUS codes (deepmodeling#7843)
* add a python script to check the code quality * update * Add cyclomatic complexity rule; raise file_too_long weight to 2 Extends tools/03_code_analysis/code_quality_score.py with two changes: - New `high_cyclomatic_complexity` rule: counts if/for/while/switch/case/ &&/|| per function body (McCabe complexity). Threshold 10, -1 per extra point, capped at 30 per file. Identifies functions that should be split. - `file_too_long` weight raised from -1 to -2 per 50-line block beyond 500 lines, reflecting the higher maintenance cost of very large files. Implementation: - `find_function_bodies()` locates function definitions with `{...}` bodies, reusing the prefix/reject logic from find_long_function_signatures so that function calls, lambdas, macros, and function-pointer typedefs are excluded. - `find_high_complexity_functions()` walks each body and counts control-flow keywords via CYCLO_KEYWORDS_RE. - Cyclomatic complexity follows McCabe: `else if` counts as two `if`, `switch` + each `case` count separately, `&&`/`||` each add 1. Scan results on source/ (1652 files, excluding test/ dirs): - Average score: 79.1 (was 82.1) - Passing rate (>=60): 1355/1652 = 82.1% - high_cyclomatic_complexity triggered: 594 functions - file_too_long triggered: 167 files Top offenders identified by the new rule: - source_hamilt/module_xc/xc_grad.cpp:28 `gradcorr` (complexity 145) - source_lcao/force_stress_lcao.cpp:69 `getForceStress` (103) - source_lcao/module_deepks/lcao_deepks_iface.cpp:63 `out_deepks_labels` (94) - source_io/module_ctrl/ctrl_scf_lcao.cpp:82 `ctrl_scf_lcao` (68) - source_estate/module_charge/charge.cpp:245 `atomic_rho` (60) * Add post-C++11 rule (-80); expand test dirs; fix string-literal false matches This commit extends tools/03_code_analysis/code_quality_score.py with one new scoring rule, expands the test-file exclusion list, and fixes a critical class of false positives for keyword-based rules. 1. New rule: post_cpp11_feature (-80, one-shot per file) The ABACUS project keeps a C++11 baseline (see AGENTS.md § Required Baseline rule 7). Any newer syntax is a compilation risk on older compilers, so a one-shot -80 deduction is applied when any of the following high-confidence, low-false-positive patterns is seen: C++14 std::make_unique<T>(...) digit separator in numeric literals (1'000'000) C++17 if constexpr (...) structured binding auto [a, b] = ...; fold expressions (args + ...), (... + args), etc. std::optional<T>, std::variant<T,U>, std::any [[nodiscard]], [[maybe_unused]] attributes C++20 concept / requires / consteval / constinit coroutine keywords: co_await, co_yield, co_return std::span<T>, std::ranges::*, std::format(...) C++23 std::expected<T,E>, std::print(...), std::println(...) Detection uses a list of (label, compiled_regex) pairs defined in POST_CPP11_PATTERNS. A single Finding is emitted per file listing all distinct features and their line numbers so the report is actionable. 2. Test directory exclusion: add "test_serial" to SKIP_DIRS The exclusion set previously contained {test, tests, test_parallel, unit_test, unittest} but missed test_serial/ under source_io and source_base; nine files leaked into score summaries. Now skipped. 3. False-positive fix: introduce strip_strings() helper strip_comments() erases comments but preserves string literals on purpose (brace-matching parsers later rely on the real quote boundaries). That meant keyword-based rules (e.g. the C++20 requires regex) matched ordinary words inside user-facing strings such as WARNING_QUIT("... eigensolver requires replicated ..."). The new strip_strings() function walks through content character by character, tracks "... " and '...' modes, and replaces every character inside quotes with a space (newlines are preserved so line numbers stay correct). find_post_cpp11_features() now runs on strip_strings(strip_comments(content)) — the double pass eliminates string-literal false matches while still catching real keywords. 4. Results on source/ (1643 files, excluding test dirs): - Avg score: 79.1 (previous scan w/ buggy version: 78.6) - Pass rate (>=60): 1348/1643 = 82.0% - post_cpp11_feature triggered on exactly 1 file after the fix: source/source_hsolver/diago_pexsi.cpp -> std::make_unique (C++14) The previous 16 files flagged as "requires (C++20)" were all string-literal false matches and are now correctly cleared. * use code quality tool to fix issues in wavefunc_in_pw files * update code_quality_score * update code_quality_score * remove C++14 code in diago_pexsi.cpp * update * update * update * delete wavefunc_in_pw because they have not been used * update makefile * remove C++14 codes in tests * remove using namespace std in psi_base.h * remove Chinese notes * remove one duplicate name of variables * fix(code_quality): exclude template type params and enum class in find_class_blocks - Reject 'class T' / 'struct T' appearing inside template parameter lists (preceded by '<' or ',' after skipping whitespace), preventing the enclosing function body from being misidentified as a class body. - Reject 'enum class' / 'enum struct' scoped enumerations by checking backwards for the 'enum' keyword with optional intervening whitespace. - Update docstring to document the two exclusion cases. Reproducers fixed: template <class T> -> no longer returns [(1, 5, 'class', 'T')] enum class Kind {} -> no longer treated as class block 'Kind' * fix(code_quality): handle trailing return type and ctor init list in fn scan Add two helpers to advance past post-parameter-list suffixes before the caller looks for function terminators (;, {, =): * _skip_trailing_return_type: advances over '-> ReturnType' including qualified and templated types such as std::vector<int> and int (*)(int). Stops at the first terminator / qualifier keyword encountered at bracket depth 0. ':' is NOT treated as a terminator so scope-resolution '::' inside the return type is preserved. * _skip_member_initializer_list: advances over ': a(x), b{1,2}' constructor member initializer lists. Distinguishes a member brace-init 'a{...}' from the actual function-body opener at depth 0 by looking at the previous non-whitespace char: when the prior char is ')', '}', ',' or ':' the '{' is the function body and scanning stops; otherwise it is a member brace-init and depth is increased normally. Refactor post-')' scanning in both find_long_function_signatures and find_function_bodies to use a loop that consumes qualifiers, the trailing-return type (via helper), and the member initializer list (via helper) in any valid order before looking for terminators. The initializer-list ':' is additionally guarded to ensure it is preceded by the closing parameter paren (prevents 'Foo::Bar()' from being treated as an init list). Reproducers fixed: auto f(int x) -> int { ... } -> now reported as body 'f' A::A(int x) : x_(x) { ... } -> now reported as ctor 'A' (not 'x_') * fix(code_quality): blank string literals before cyclomatic complexity scan find_high_complexity_functions previously applied CYCLO_KEYWORDS_RE on top of strip_comments() output only: control-flow words inside string and character literals (e.g. 'const char* msg = "if not ok";') were counted as genuine if/for/while/switch/case/&&/|| tokens. Pipe strip_comments() through strip_strings() before slicing each function body. Since both helpers are position-preserving (replace content with spaces instead of shortening), the absolute offsets returned by find_function_bodies remain valid for the blanked text. Update docstring to document the string/char-literal blanking and the rationale (user-facing messages often mention control-flow keywords and should not inflate the complexity score). Reproducer fixed: const char* text = "if if if (x11)"; Previously reported as complexity 11; now correctly 0. * fix(code_quality): distinguish C++14 digit separators from char literals Both strip_comments() and strip_strings() previously switched into character-literal mode on every occurrence of the single-quote character, which caused the two inner quotes in to be interpreted as the start of and character literals respectively. The number was subsequently either kept verbatim (comment-strip) or blanked out (string-strip), so the advertised digit-separator C++14 detector regex never matched it at all. Add a small _is_digit_separator(content, quote_pos) helper that returns True when the characters immediately before and after a both belong to the set of characters that may appear inside a numeric literal (digits, hex letters a-f/A-F, base/type suffix letters uUlLbBxXoO, and floating-point '.'). A that satisfies this check is passed through without toggling the in_char state in either scanner. Apply the check in both strip_comments() and strip_strings() before the in_char = True transition, and update both docstrings to mention the digit-separator behaviour. Character literals such as 'x', '\'' and '\n' continue to be handled correctly because their surrounding chars are not numeric-adjacent in the required sense. Reproducer fixed: int value = 1'000'000; find_post_cpp11_features used to return no matches; now reports the digit-separator finding on line 1. * fix(code_quality): distinguish declarations from call sites in param-count scan find_long_function_signatures previously accepted any candidate with a non-empty prefix text before name(...) whenever the terminator was ';' or '='. That produced duplicate reports for call sites such as: int f(int a, int b, int c, int d, int e, int f, int g, int h); int g() { return f(1, 2, 3, 4, 5, 6, 7, 8); } because the call's prefix ('return') is non-empty even though the candidate is a call expression. The same false positives hit calls inside if() conditions, argument lists, assignment RHS, throw expressions, casts, sizeof(...), coroutine keywords, etc. Introduce a dedicated _is_declaration_prefix(prefix_stripped) helper that rejects a ';' or '=' candidate when: - the prefix is empty; - the last two chars form an expression-only operator ('&&', '||', '**', '*&', '&*', '->') — single '*' and single '&' are still accepted because they are valid pointer/reference qualifiers on the return type; - the last char belongs to an expression/argument-list punctuation set ('=', '+', '-', '/', '%', '|', '^', '~', '!', '<', '>', '?', '(', '[', '{', ',', '.', ';', ':'); note '*' and '&' are NOT in this set for the reason above; - the trailing identifier token belongs to a STATEMENT_CONTEXT_KEYWORDS set that extends NON_FUNCTION_KEYWORDS with co_await/co_return/ co_yield, typeid/noexcept/alignof/alignas/decltype, the four named casts, and the C++ alternative operator tokens. Replace the old 'not prefix_stripped' one-liner in find_long_function_signatures with a call to the helper. Constructors, destructors and function bodies that terminate with '{' are not subjected to the check and keep the existing empty-prefix acceptance. Known trade-off: return types decorated with double-pointer 'int**' are intentionally skipped because the tail '**' cannot be told apart from the expression-level multiplication operator; this is a rare shape in practice and false-positive suppression is prioritized. Reproducer fixed: declaration + 'return f(1..8);' used to report f twice; now only the declaration line is reported. Assignment/if-condition/ argument-list calls no longer generate false positives, while real declarations ('virtual int calc(..) = 0;', 'const int* factory(..);', 'ns::Class::method(..) { }') still count correctly. * fix(code_quality): cancel smart-pointer owned new in unpaired_new heuristic The file-level leak heuristic used a straight `new_count - delete_count` difference, which could not see ownership transfers that never produce a literal `delete` keyword. The C++11 idiom that replaces std::make_unique (which only arrived in C++14) therefore looked like a leak: std::unique_ptr<Foo> value; value.reset(new Foo()); // new_count=1, delete_count=0 before fix Introduce OWNED_NEW_RE, a single alternation regex covering the common smart-pointer ownership patterns: * p.reset(new T(...)) and p->reset(new T(...)) (plus operator= forms) * unique_ptr<T>/shared_ptr<T>/scoped_ptr/auto_ptr local variables constructed directly with (new T(...)) next to the declarator * a loose fallback for make_unique/make_shared/allocate_shapes that somehow end up wrapping a visible `new T` inside their call Inside analyze_file, compute owned_new_count with OWNED_NEW_RE, then calculate cancelled_new = delete_count + owned_new_count before deriving unpaired_new. raw_new_count deliberately stays equal to new_count and ignores the ownership cancellation: raw_new_keyword is the stylistic penalty for writing `new` instead of using make_unique/make_shared factories, so reset(new Foo) and unique_ptr<T>(new T) correctly still contribute to that count. Document both sides of the rule split in a longer inline comment so future readers can understand why unpaired_new and raw_new_count may diverge on modern C++ files. Side note discovered during verification, NOT addressed in this patch: the existing DELETE_EXPR_RE regex always requires a `[` token after the keyword, which means plain scalar `delete p;` expressions are never counted today. This pre-existing bug is outside the scope of the current comment and is tracked separately. Reproducer fixed: value.reset(new Foo()); previously raised unpaired_new to 1; now the new is cancelled by OWNED_NEW_RE and unpaired_new stays at 0. The corresponding raw_new_keyword deduction of 1 remains intact because the code still bypasses std::make_unique / std::make_shared. * fix(code_quality): skip multi-line using/typedef continuations in member scan is_public_member_var and is_static_member_var are single-line heuristics. When a using/typedef/template declaration spans multiple lines, the continuation half (e.g. ' = std::vector<int>;') looks exactly like a member variable declaration in isolation: it ends with ';', has no parens or braces, doesn't start with a bad prefix, and contains identifier characters. This produced bogus public member findings inside structs like struct A { using value_type = std::vector<int>; }; and inside real headers such as sto_tool.h. Add _mark_continued_decl_lines(class_lines): a per-class-body scan that returns the set of 0-based line indices belonging to a multi-line declaration started by a previous line. The scanner tracks a stack of (depth_at_starter, starter_idx) pairs for any class-body line whose stripped form begins with one of _MULTILINE_DECL_STARTERS (using, typedef, template, typename, namespace, extern, friend) and does NOT contain ';'. Subsequent lines are marked as continuations until a ';' at the starter's brace-depth closes the declaration. The starter line itself is never marked; the terminating ';' line IS marked so the per-line heuristics never see it standalone. Wire the marked set into analyze_class_blocks: * pass 0 computes continued_idxs once per class body. * pass 1 (member_var_names collection) skips continued lines so bogus names like '=' or 'std::vector<int>' do not pollute the member/local conflict set. * pass 2 public-member rule skips continued lines before calling is_public_member_var. * pass 2 static-member rule skips continued lines before calling is_static_member_var. The starter keyword list intentionally uses bare forms (no trailing space) so that authors who wrap right after the keyword (e.g. 'typedef\n int myint;') are still detected. Reproducer fixed: using value_type = std::vector<int>; used to report 'public member in struct A: = std::vector<int>;'; now the continuation is suppressed and no false positive is raised. Real members declared on their own line ('int counter;', 'IntVec data;') and static members are still detected. Note (not addressed here): classes whose entire body is squashed onto the header line (e.g. 'struct B { int x; };') were already skipped by the depth-1 walk before this change; that pre-existing behaviour is unchanged. * fix(code_quality): exclude arrow member access from uppercase constant rule UPPERCASE_CONST_RE used a (?<![.:]) lookbehind that only excluded the '.' and ':' member-access prefixes. The '->' (arrow) form of member access was missing: 'ptr->UPPER_MEMBER' still matched UPPER_MEMBER even though the semantically equivalent 'obj.MEMBER' and 'Type::MEMBER' were already excluded. This produced different scores for equivalent code depending on whether a pointer or a value/member-access was used. Add '>' to the lookbehind character class so that the character immediately preceding the identifier is now '.' | ':' | '>'. '>' is a literal inside a Python regex character class and requires no escaping. The lookahead (?![.:]) is intentionally left unchanged: the token that follows an arrow member access is usually ';', '(', '=', or whitespace, never '.' or ':', so mirroring the '>' there would be dead weight. This keeps the rule symmetric with how '.' and ':' were already handled (prefix-only exclusion). Reproducer fixed: value = ptr->UPPER_MEMBER; used to match UPPER_MEMBER (1 deduction); now excluded, matching the existing behaviour for obj.MEMBER and Type::MEMBER. Real constants declared on their own line (MY_CONSTANT, GLOBAL_MAX, RED/GREEN/BLUE enum values, #define FOO macros, function-argument constants such as func(MAX_VAL)) are still detected. * fix(code_quality): do not deduct for public members in struct bodies The public_member_variable rule previously applied uniformly to both class and struct bodies, treating any public data member as a code-quality finding regardless of the enclosing type. In C++ a struct has public access by default and public data members are the idiomatic shape for POD aggregates, value types, configuration data, and mixin tags; penalising them charges the author for writing legitimate, intended C++. Gate the finding in analyze_class_blocks on kind == 'class'. struct bodies — including struct members that appear inside an explicit 'public:' access block — no longer produce public_member_variable findings. class bodies keep the existing behaviour: public data members in a class continue to be deducted because the author of a class is expected to encapsulate state. Update the inline comment to explain the rationale so future readers understand why struct and class are treated differently. Reproducer: struct A { int counter; double value; }; used to report 2 findings (counter, value); now 0. class B { public: int counter; double value; }; still reports 2 findings. * refactor(hsolver_test): replace unique_ptr heap alloc with stack vars in bpcg test The alpha/beta scaling constants passed to ModuleBase::gemm_op inside hpsi_func were heap-allocated via std::unique_ptr<T>(new T(...)) and then re-exposed through .get(). GEMM only reads these values (const T* alpha / const T* beta in gemm_op::operator()), so heap allocation is unnecessary — the lambda performs a fresh new/delete pair on every call for no semantic benefit. Replace with stack-local const T one(1.0) / const T zero(0.0) and pass &one / &zero directly. The result is fully C++11-compatible (indeed C++98-compatible), shorter, and removes the only std::unique_ptr use in the file, so <memory> is no longer needed even indirectly through this translation unit's own code. Verified by building the MODULE_HSOLVER_bpcg test target: make -j 30 MODULE_HSOLVER_bpcg -> [100%] Built target MODULE_HSOLVER_bpcg --------- Co-authored-by: abacus_fixer <mohanchen@pku.eud.cn>
1 parent 12ecc79 commit c9bae03

18 files changed

Lines changed: 1939 additions & 553 deletions

File tree

source/Makefile.Objects

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -470,7 +470,7 @@ OBJS_PSI_INITIALIZER=psi_base.o\
470470
psi_init_atomic.o\
471471
psi_init_atom_rand.o\
472472
psi_init_nao.o\
473-
psi_init_nao_random.o\
473+
psi_init_nao_random.o
474474

475475
OBJS_PW=fft_bundle.o\
476476
fft_cpu.o\
@@ -691,7 +691,6 @@ OBJS_LCAO=evolve_elec.o\
691691
center2orb_orb21.o\
692692
center2orb_orb22.o\
693693
record_adj.o\
694-
wavefunc_in_pw.o\
695694

696695
OBJS_MODULE_RI=conv_coulomb_pot_k.o\
697696
exx_abfs-abfs_index.o\

source/source_basis/module_ao/test/orb_test.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -213,7 +213,7 @@ void test_orb::set_single_c2o(int TA, int TB, int LA, int NA, int LB, int NB)
213213
{
214214
this->test_center2_orb11[TA][TB][LA][NA][LB].insert(std::make_pair(
215215
NB,
216-
std::make_unique<c2o>(ORB.Phi[TA].PhiLN(LA, NA), ORB.Phi[TB].PhiLN(LB, NB), OGT.MOT.pSB, Center2_MGT)));
216+
std::unique_ptr<c2o>(new c2o(ORB.Phi[TA].PhiLN(LA, NA), ORB.Phi[TB].PhiLN(LB, NB), OGT.MOT.pSB, Center2_MGT))));
217217
}
218218
double test_orb::randr(double Rmax)
219219
{

source/source_hsolver/diago_pexsi.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ DiagoPexsi<T>::DiagoPexsi(const Parallel_Orbitals* ParaV_in,
3434
}
3535

3636
this->ParaV = ParaV_in;
37-
this->ps = std::make_unique<pexsi::PEXSI_Solver>();
37+
this->ps.reset(new pexsi::PEXSI_Solver());
3838

3939
this->DM.resize(this->nspin_dm);
4040
this->EDM.resize(this->nspin_dm);

source/source_hsolver/kernels/cuda/diag_cusolvermp.cu

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ Diag_CusolverMP_gvd<inputT>::Diag_CusolverMP_gvd(const MPI_Comm mpi_comm,
5858
const int nacols,
5959
const int* desc)
6060
{
61-
// 构造函数的实现
61+
/// constructor implementation
6262
this->cblacs_ctxt = desc[1];
6363
this->nFull = desc[2];
6464

source/source_hsolver/test/diago_bpcg_test.cpp

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -136,20 +136,18 @@ class DiagoBPCGPrepare
136136
const std::vector<T> &h_mat = DIAGOTEST::hmatrix_local;
137137
auto hpsi_func = [h_mat, dim](T *psi_in, T *hpsi_out,
138138
const int ld_psi, const int nvec) {
139-
auto one = std::make_unique<T>(1.0);
140-
auto zero = std::make_unique<T>(0.0);
141-
const T *one_ = one.get();
142-
const T *zero_ = zero.get();
139+
const T one(1.0);
140+
const T zero(0.0);
143141

144142
base_device::DEVICE_CPU *ctx = {};
145143
// hpsi_out(dim * nvec) = h_mat(dim * dim) * psi_in(dim * nvec)
146144
ModuleBase::gemm_op<T, base_device::DEVICE_CPU>()(
147145
'N', 'N',
148146
dim, nvec, dim,
149-
one_,
147+
&one,
150148
h_mat.data(), dim,
151149
psi_in, ld_psi,
152-
zero_,
150+
&zero,
153151
hpsi_out, ld_psi);
154152
};
155153
const int ndim = psi_local.get_current_ngk();

source/source_hsolver/test/diago_pexsi_test.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -160,7 +160,7 @@ class PexsiPrepare
160160
std::cout << "nrow: " << hmtest.nrow << ", ncol: " << hmtest.ncol << ", nb: " << nb2d << std::endl;
161161
}
162162

163-
dh = std::make_unique<hsolver::DiagoPexsi<T>>(&po, PARAM.input.nspin, nlocal, PARAM.input.nelec);
163+
dh.reset(new hsolver::DiagoPexsi<T>(&po, PARAM.input.nspin, nlocal, PARAM.input.nelec));
164164
}
165165

166166
void distribute_data()

source/source_io/module_unk/berryphase.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,7 @@ void berryphase::set_kpoints(const K_Vectors& kv, const int direction)
113113

114114
nppstr = mp_x + 1;
115115
}
116-
else if (direction == 2) // 计算y方向
116+
else if (direction == 2) /// compute the y direction
117117
{
118118
const int num_string = mp_x * mp_z;
119119

@@ -163,7 +163,7 @@ void berryphase::set_kpoints(const K_Vectors& kv, const int direction)
163163

164164
nppstr = mp_y + 1;
165165
}
166-
else if (direction == 3) // 计算z方向
166+
else if (direction == 3) /// compute the z direction
167167
{
168168
const int num_string = mp_x * mp_y;
169169

source/source_io/module_wannier/to_wannier90_pw.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@
1414
#include "source_base/matrix.h"
1515
#include "source_base/matrix3.h"
1616
#include "source_cell/klist.h"
17-
#include "source_lcao/wavefunc_in_pw.h"
17+
#include "source_basis/module_pw/pw_basis_k.h"
1818
#include "source_psi/psi.h"
1919

2020
class toWannier90_PW : public toWannier90

source/source_lcao/CMakeLists.txt

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,6 @@ if(ENABLE_LCAO)
6565
center2orb_orb11.cpp
6666
center2orb_orb21.cpp
6767
center2orb_orb22.cpp
68-
wavefunc_in_pw.cpp
6968
)
7069

7170
add_library(

source/source_lcao/module_deltaspin/spin_constrain.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -825,7 +825,7 @@ void SpinConstrain<TK>::print_Mi(std::ofstream& ofs_running)
825825
* @par Typical values
826826
* - Well-converged SCF: lambda ~ 0.01-1 eV/uB
827827
* - Strongly constrained: lambda ~ 1-10 eV/uB
828-
* - Diverging SCF: lambda growing without bound (check target_mag合理性)
828+
* - Diverging SCF: lambda growing without bound (check target_mag validity)
829829
*/
830830
template <typename TK>
831831
void SpinConstrain<TK>::print_Mag_Force(std::ofstream& ofs_running)

0 commit comments

Comments
 (0)