Add a python script to test the code quality of ABACUS codes - #7843
Merged
Conversation
added 3 commits
August 20, 2026 22:27
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)
mohanchen
added
the
Compile & CICD & Docs & Dependencies
… 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.
mohanchen
added
the
Tests/Examples
AsTonyshment
left a comment
AsTonyshment
left a comment
Collaborator
There was a problem hiding this comment.
Keeping the source compatible with C++11 is a reasonable goal, especially for older HPC systems (even though the current CMake configuration uses at least C++14 when ENABLE_PEXSI, ENABLE_LIBRI, or BUILD_TESTING is enabled, when USE_CUDA is used with CUDA Toolkit < 13, or when 1.5.0 <= Torch_VERSION < 2.1.0; it uses at least C++17 when USE_CUDA is used with CUDA Toolkit >= 13, or when Torch_VERSION >= 2.1.0).
However, my coding agent tested the parser with many small, valid C++11 examples and found several reproducible false positives and missed cases. I have described these cases, together with a few suggestions of my own about the related C++ changes and scoring rules, in the inline comments. I hope they are helpful :)
added 6 commits
August 25, 2026 07:25
…d_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'
…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_')
… 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.
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.
…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.
…ristic
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.
…ber 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.
…t 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.
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.
… 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
AsTonyshment
approved these changes
Aug 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add a python script to test the code quality of ABACUS codes