Skip to content

feat(const-param): experimental Clang check of const suggestions, version 2 - #239

Draft
SizzleUnrlsd wants to merge 159 commits into
mainfrom
217-t-v2
Draft

SizzleUnrlsd wants to merge 159 commits into
mainfrom
217-t-v2

Conversation

@SizzleUnrlsd

@SizzleUnrlsd SizzleUnrlsd commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

Part of #217. Fixes #237, through #238's d711321, carried here.

The second version of the experimental Clang check of ConstParameterNotModified suggestions (--experimental-const-suggestion-check). #221 stays open as a draft and is not to be merged: it is the reference of the first version (217-t-v1 at 2a5b0c1).

This branch carries d711321 from #238 (#237), merged in 46c036f, and 6a91a79 and 270955e from #241 (#240), merged in 4fd0dae: its unit tests pass on Linux arm64, and its messages name C++23 explicit object members on macOS 14 too. #238 is closed as superseded by this PR; #237 stays open until this PR is delivered.

Depends on #241. The fallback to LLVM's demangler (#240) changes the output without --experimental-const-suggestion-check: symbols that the C++ runtime's demangler predates are now demangled. That change belongs to #241 and is reviewed and merged there on its own; it is not an exception that #239 makes to its rule that the output without the option stays the same. #239 carries 6a91a79 and 270955e only because const-suggestion-explicit-object.cpp needs them on macos-14. #258, now in main, replaces that job with macos-15: this PR's CI, which runs on its merge ref, now builds on macos-15, where the suite passes, with the fallback present. The order is #258, then #241, then #239: #258 is in main, and #241 has been updated with main (3027b09). #239 is not merged before #241; once #241 is in main, #239 is updated with main, and its diff no longer contains the change.

Status

Draft, not to be merged. Stages 1 to 3, the final phase, the fixes of the final review and those that followed its targeted review are done; 2318ea2 merges main at f71bdac (#246) and is validated. 8658cd0 adds the fixes of the two defects that were open at 2318ea2 (below), 8f875a7 formats them, and c84b959 adds a test of a defining file that imports a module. c84b959 is validated (below). One criterion is not met: the cost. In the campaigns run up to 42fa604, before #246, the option added about 22 to 31 % to the self-analysis on 4 CPUs, depending on the campaign; at 2318ea2, on its own sources, it adds +30.1 % (below, with the measurement reserves). The cost is not measured again after 2318ea2. The plan asks for at most 10 %; this overrun is not accepted.

Two defects that were open at 2318ea2 are fixed at 8658cd0. In both, a suggestion was reported checked while its rewrite breaks the program or adds a constraint violation. Each fix follows its test, which fails at 2318ea2.

  1. A callback passed through ?: (test 5dcbae0, fix af1edbe).
    • callout_enumerate_function_8's cb and callout_data, in PCRE2's pcre2test, were reported checked. The function reaches pcre2_callout_enumerate through a conditional, c ? f1 : f2.
    • With the rewrite, the two operands point to incompatible function types. C makes that a constraint violation (C11 6.5.15p3); Clang reports it only as -Wpointer-type-mismatch, a warning that the check did not track. Passed directly, the same rewrite is an error, and such suggestions were already removed.
    • That group is now tracked like the others: enabled, and kept a warning under -Werror.
    • check_const_suggestion_conditional_between_callbacks covers the default warnings and -Werror -Wno-pointer-type-mismatch, with a conditional between const T * and T *, which C allows, as its witness. In PCRE2, both suggestions are now removed for that warning (Corpora at 8658cd0, below).
  2. A file that imports a module (test 10adf29, fix 8658cd0).
    • The check tells whether a file of the compilation database sees the function from that file's preprocessing, which does not read a module that the file imports.
    • A file that got the function's declaration from a Clang header module (-fmodules), and never named the function in its own text, was not judged. In a minimal C++ case on macOS, the suggestion was reported checked, while the rewritten program no longer links (D17 missed).
    • The scan now records an import: an #include that becomes one, a C++20 header unit, an import declaration, or a module implementation unit, which imports its interface.
    • A file that imports a module, and does not see the change otherwise, keeps the suggestion not checked. The exception: the rewrite changes only the defining file, and the function is not a free function with external linkage. A refutation by another file still wins.
    • check_const_suggestion_module_import covers, for a function f declared in a header, each of the four forms, a precompiled header as a witness (judged, D17, as before), two refutations, and a static function s, which stays checked in every case. The exception's two conditions are not tested separately: in every case, f has external linkage and is declared in a header that the rewrite changes, and s has neither.

Limits on modules, kept:

  • The check does not rebuild modules with the rewritten files. A file that names the function and imports a Clang header module that declares it is judged: in two cases run on macOS arm64, outside the suite, at 2318ea2 and again at 8658cd0, the suggestion is not checked.
  • A file that gets the declaration through a precompiled header (-include-pch) is judged, as with a plain include, when the header that the PCH was built from is still there, unchanged.
  • Under -fmodules, an #include of a header that a module map names, system headers included, counts as an import. In such a database, a suggestion is not checked as soon as one file imports a module and does not see the change otherwise, unless the rewrite changes only the defining file and the function is not a free function with external linkage; a refutation by another file still wins. This rule applies to every free function with external linkage, and to every function whose rewrite changes a header, such as a method declared in its class's header. For example, with a file that imports the module holding the rewritten header and does not use the function, the suggestion was checked at 2318ea2 and is not checked at 8658cd0.
  • A defining file that imports a Clang header module: one targeted test, check_const_suggestion_defining_file_imports_a_module (c84b959), covers one case. The module's header uses the function in a way that the rewrite breaks. Because the module keeps the old declaration, the suggestion is not checked; without -fmodules, the same files refute the rewrite. No code changes with this test: it pins that verdict and its reason.
  • Not covered: a defining file that is a C++20 module unit, or that imports a named module or a header unit; a module whose declaration of the function the defining file does not see; a module built before the analysis from other sources than those on disk.

Measurement reserves (the campaign at 2318ea2, on one Linux x86_64 machine):

  • The clock. The machine's monotonic clock ran about 8 % fast, and its wall clock, which times the passes, was stepped back about 2.5 s every 32 s, very likely by systemd-timesyncd; the self passes' samples, 5 s apart, show these steps throughout each pass. If each step brought the wall clock back to real time, which measurements taken after the passes support without proving, a pass reads within 2.5 s of its real duration, and the +30.1 % lies between +29.3 and +30.8 %. The --timing figures read about 8 % high; the corpora's short passes are read in CPU time, which is real.
  • The load. The second passes of both binaries with the option ran under load from outside the container: a load average of 12.8 during 42fa604's and 4.3 during 2318ea2's, against 2.0 to 2.9 on average during the other passes, both passes without the option among them. Leaving out 2318ea2's second pass, the option adds +28.9 % against the mean of the two passes without it; first pass against first pass, +27.5 %.
  • One campaign. 42fa604 and 2318ea2 differ by 0.1 % (1041.9 and 1043.2 s), in one first pass each, within the passes' noise (2.3 % between the two passes without the option): this does not show that their costs are equal. The earlier campaigns analyzed their own head's sources and are not compared with this one.
  • Under each of these readings, the +10 % criterion is not met.

Reserves:

What changes from the first version

The first version removed a suggestion whenever the rewritten code differed, whatever the difference. This version gives each suggestion one of three verdicts, from what Clang compiles before and after the rewrite:

  • verified: kept, with the checked ... line;
  • refuted: removed. The rewrite adds an error or a tracked warning, or changes what a call or a reference targets, what a method overrides, an if constexpr condition or a _Generic selection;
  • indeterminate: kept, with not checked with Clang: <reason>. Examples: a file that cannot be checked, a rewrite that changes more than it plans, facts that cannot be compared one to one, symbols that cannot be paired one to one, a dependency that the rewrite renames and the file only declares, a difference of the generated code that the facts do not explain, a rewritten file that does not compile without an error that the rewrite adds, a file that declares the function itself or casts its address, a file that imports a module and does not see the change otherwise, a possible collision between files.

How it runs:

  1. The defining file's compilation plans each suggestion: the function, found by its symbol, the declarations to rewrite, and the type tried.
  2. Each file that sees the change is compiled as it is and with the rewrite.
  3. The symbols that the rewrite renames are paired from compilation facts only: variants, inheriting constructors, nested symbols, instantiation contexts, and (step 5) the entities nested in paired functions: members of their lambdas and local classes, specializations whose arguments or type hold them, implicit members of the classes that hold them. The comparator takes these pairs; its name heuristics are gone.
  4. Collisions are judged once every file is, and only a refutation stops the judgement.

With --verbose, the trace gives each verdict's grounds. The README section "Experimental: const parameter suggestions checked with Clang" describes the check and its limits. Of the limits on modules (Status, above), it states the rule for a file that imports a module and does not see the change; the others are stated only here. Without the option, nothing changes.

Stage 1: the first version's defects

First version This version Test
D1 vtt suggested, x (C2) removed no vtt suggestion (#233), x verified const-suggestion-virtual-base.cpp
D2 a member operator used in another file: removed verified check_const_suggestion_operator_used_elsewhere
D3 a C static function and an external homonym: removed verified, both ways check_const_suggestion_homonyms_across_files
D4 an overload defined in another file: checked indeterminate, the link assumption stated check_const_suggestion_overload_defined_elsewhere
D5 a macro argument that writes two parameters: removed indeterminate const-suggestion-macro-shared-type.c
D7 an inheriting constructor: removed verified const-suggestion-inheriting-constructor.cpp
D11 \ and spaces in a #define: the macro body rewritten, removed indeterminate check_const_suggestion_line_endings
D14 a correspondence that is not one to one: removed indeterminate const-suggestion-deduction-refused.cpp, expectation changed
D15 an explicit cast of the address: removed indeterminate, unless an error or a fact refutes check_const_suggestion_cross_tu, check_const_suggestion_compilation_database, expectations changed; check_const_suggestion_cast_in_the_defining_file
D16 sizeof(__PRETTY_FUNCTION__): removed indeterminate const-suggestion-printed-signature.c, expectation changed
D17 a redeclaration with the old type in another file: removed indeterminate, the link assumption stated the two checks of D15, expectations changed

Other tests added cover:

  • collisions: a merged declaration, a strong and a weak definition, a function that a shared header defines;
  • the units: one configuration per file, one compilation per file;
  • the scope and the inspection: the scope after the rewrite, a fatal error, an invalid declaration, two functions of one macro invocation;
  • the facts: facts without a change of code, repeated and unpaired facts;
  • the trace.

The comparator's unit tests take the established pairs. Each test failed first, on the first version, on an earlier state of this branch, or on a mutant of the code.

Stage 2: the remaining defects of the catalogue

Defect Before Now
D6: a compile command with -Werror that disables a tracked warning group not checked: the file does not compile as it is the tracked groups keep the level Clang gives them by default, warnings under -Werror and default errors as errors
Review Focus 5: -Werror=<group> on a tracked group removed for an error removed for the warning
D8: a C function that a C++ file declares through an extern "C" header not checked each declaration is compared with the candidate as its own unit's language writes it
D18: declarations that differ from the definition in a top-level qualifier, both directions not checked each declaration is compared with the candidate of its own type, qualifiers kept
D12: an rvalue reference the message offers passing by value, which is not checked consider passing by const reference (...); the "checked" line says passing by value is not checked
D13: int *const p, only read displays const int *p, checks const int *const p displays and checks const int *const p
D10: a qualifier written apart from a pointer typedef (const IntPtr p) rewritten const const int *, not checked rewritten const int *const p, verified; a qualifier written by a macro keeps the suggestion as not checked

The type displayed comes from the plan's candidate only, written as Clang writes it, with struct, union and enum where C needs them; a type without a name stays <anonymous type>. A unit test checks that a rewrite losing the pointer's own const is caught, and fails with a comparison that ignores top-level qualifiers. The corresponding limits are removed from the README.

Each test was added after its failure was shown. Over every const-suggestion fixture and the first version's review cases, with and without the option, only the planned lines change; no suggestion disappears, and the output without the option is unchanged.

Stage 3 and the final phase

Stage 3, cost. Measured first (Task 3.1): the 38 compilations of T about 270 s summed over the chunks (front-end about 166 s, inspection 87 s, CodeGen 17 s), the correspondences 80 to 90 s, the scans 31 s; the copies of the scans 2.7 s and the preparation of the commands under 1 s. 48f3ec1 builds the correspondences' index once per call (correspond 49.5 -> 23.0 s, same pairs). The other leads were measured or prototyped and not kept: a precompiled preamble (a net loss, and a crash), sharing the pipeline's compilation (not recommended); the shared scans and a single construction of the Clang invocation stay as they are, their measured cost being below the noise.

Corpora (this repository's sources, zlib, tinyxml2, PCRE2; Linux x86_64): no confirmed valid suggestion disappears against the output without the option. The comparison with the first version found two classes of difference, fixed here:

  • D19 (7010ff6): a declaration whose type is deduced from a call that the rewrite makes invalid (publishSingleStoreSlots). Step 3 now judges a type's written form, its deduced part being left to the later steps, and the types that the edits write outside the planned parameters, default arguments included. The suggestion is refuted for its error, as in the first version.
  • D20 (7511f50): six suggestions of this repository (parallelFor x2, build x2, isRecursiveCallee x2) were not checked, because the specializations that lambdas and local classes nested in a paired function parameterize had no pair. Step 5 of the correspondences identifies them from Clang's facts, read before the AST goes: the members of those nested types, the specializations whose arguments or type hold them, and the implicit members of the classes that hold them (libstdc++'s _Vector_impl and _Deque_impl destructors among them). In a key only the nested types go through the pairs; every other part must match; a key held by more than one symbol on either side gives no pair, and the ABI variants stay apart. The six are now verified, on Linux and macOS.

Against the product before these two fixes, at bed265d: only those six change (not checked -> checked); the other 92 suggestions keep their verdict and reason (src/ 4/6/12 -> 10/0/12 verified, not checked, removed; zlib 16/0/10, tinyxml2 0/0/6, PCRE2 10/31/3 unchanged; PCRE2's 31 are declarations written by a macro). At 42fa604 against bed265d, the 98 suggestions keep their verdict and reason (final review, below).

The 19 units of #215. The comparisons up to 42fa604 left out the units on which the analysis did not finish, as the first version's measurements did: 5 of zlib's 37 (crc32.c, deflate.c, infback.c, inffast.c, inflate.c) and 14 of PCRE2's 35 (pcre2_auto_possess.c, pcre2_compile.c, pcre2_compile_class.c, pcre2_convert.c, pcre2_dfa_match.c, pcre2_find_bracket.c, pcre2_match.c, pcre2_pattern_info.c, pcre2_script_run.c, pcre2_study.c, pcre2_valid_utf.c, pcre2_xclass.c, pcre2grep.c, pcre2test.c). Analyzed alone without the option, before #246, each ran past a 120 s limit. At 2318ea2 (Linux x86_64), each was analyzed alone again under the same limit: without the option, 18 end in at most 7.3 s, and pcre2_match.c again runs past 120 s (with a limit of 1800 s, it ends in 254 s); with the option, the 18 end in at most 10.6 s.

Corpora at 2318ea2, with every unit (Linux x86_64; one binary, without and with the option): each source file of the compilation databases is analyzed once, zlib's 18, tinyxml2's 2 and PCRE2's 34, the generated pcre2_chartables.c left out as before (the databases list 37 and 35 entries). Every run exits 0, and two verbose passes give the same verdicts. The 76 suggestions of the earlier comparisons keep their verdict and reason. The 54 new ones all come from the 19 units:

  • 26 verified: functions of one file (static in PCRE2, local in zlib) that no other unit declares. Two of them were false "checked": callout_enumerate_function_8's cb and callout_data (defect 1, above), removed at 8658cd0 (below);
  • 25 removed. 15 are parameters of callbacks: each callback is passed where a function pointer whose parameter has no const is expected, and with the rewrite its type no longer converts, an error (pcre2test's callouts, my_malloc, my_free, jit_callback, stack_guard, pcre2grep's callout). The other 10 are parameters whose value, or a pointer reached through it (P->ovector), is returned as, stored in, or used to initialize a pointer without const, a dropped qualifier;
  • 3 not checked: pcre2_compile_8, pcre2_dfa_match_8 and pcre2_match_8, whose declarations a macro definition of pcre2.h writes, as for PCRE2's 31 earlier ones.

Corpora at 8658cd0 (Linux arm64, Docker, Ubuntu 24.04, LLVM 20.1.8; the campaign's Linux x86_64 machine cannot be reached for now, and the CI does not run the corpora). 2318ea2 and 8658cd0 were each built there and run once over every unit, with the option and --verbose, in the chunks and with the flags of the campaign.

  • 2318ea2 gives there the 130 verdicts it gave on Linux x86_64, reasons included.
  • Against it, 126 are unchanged at 8658cd0. The 4 that change are all in pcre2test:
    • callout_enumerate_function_8's cb and callout_data: from checked to removed, "gains a checked warning: pcre2test_inc.h:1800: pointer type mismatch".
    • callout_enumerate_function_void_8's cb and callout_data, the conditional's other operand: still removed, but now for that warning. They were removed for an error at pcre2test_inc.h:6969, where the function is passed directly. The check cites the first error or tracked warning that the rewrite adds, and Clang reports the warning first.
  • PCRE2 now shows 65 suggestions with the option, against 67: verified, removed and not checked are 31, 30 and 34, against 33, 28 and 34.
  • zlib (19 verified, 10 removed, 0 not checked) and tinyxml2 (0 verified, 6 removed, 0 not checked) are unchanged.
  • The analyzer's code at c84b959 differs from 8658cd0's by whitespace only (on macOS, the binary is the same file), so these verdicts stand for c84b959.

Tests. For D19: check_const_suggestion_deduced_type_error, check_const_suggestion_written_part_outside_the_plan, check_const_suggestion_default_argument_outside_the_plan. For D20: six end-to-end checks (check_const_suggestion_p2_*; five failed at 5167998), and the unit tests testConstSuggestionNestedCorrespondences, testConstSuggestionNestedIdentities, testConstSuggestionImplicitMemberIdentities. Each was proven against mutants of the code: 12 of 13 are caught. The one that survives drops the kind and the name of an implicit member: on every real case found, its variant or its type already separates it, so they are protected only together with them, as are the name, static flag and qualifiers of a nested type's member (spec §5.7).

Cost (§8, self-analysis, Linux x86_64, 4 CPUs, alternated passes): about +24 to +25 % in a 6 GiB container (first version +14.6 % in the same conditions), +25 to +31 % in a 10 GiB one; at bed265d against the product: +27.5 % and +25.3 %. Step 5 adds about 1 to 2 % to the wall time, within the passes' noise. After the final review's fixes, in one 10 GiB campaign for both: without the option 806.5 s, bed265d +22.5 %, 42fa604 +22.2 %. The two heads differ by -0.2 % in wall time and -0.7 % in CPU, within the noise: the final review's fixes do not lower the cost, and the gap with the earlier +27.5 % of the same bed265d is the variation between campaigns. At 2318ea2, in one 10 GiB campaign on one binary, on 2318ea2's sources: without the option 809.0 s, with it 1052.2 s, +30.1 % in wall time, +17.0 % summed over the chunks and +14.7 % in CPU. Wall time grows more because the option's cost falls mostly on the three chunks that one of the two jobs runs in turn (+243.2 s, against +30.2 s for the other job's chunk). On these sources, 42fa604 and 2318ea2 take 1041.9 and 1043.2 s in their first passes, their only passes without --verbose that ran without the outside load. The measurement reserves (Status, above) give the readings without the loaded passes, and the clock's bound under its condition. The earlier campaigns analyzed their own head's sources, so their figures are not compared with these. On the corpora, on the same binary, in CPU: zlib +126 %, tinyxml2 +8.3 %, PCRE2 +5.5 %. PCRE2's time is almost all the chunk that holds pcre2_match.c (about 300 s without the option); without that unit, on another machine, the option added +126 %. The +10 % criterion is not met; the overrun is not accepted.

Final review

A fresh review of the whole branch at bed265d found two critical defects, both reproduced end to end, one important point besides the dependency on #241 (I1, above), and seven minor ones. 60a9828 adds the tests first: at that commit, both checks and the two expectations of the unit test fail, and the witnesses pass. Each later commit builds without warning.

Finding Before Now Reproduced Test
C1, critical (3266376) the rewrite changes the specialization that the function calls, h<int *> to h<const int *>; h is only declared in the file, and another file instantiates h<int *>: checked, while the rewritten code fails to link not checked, naming h<int *>; with h defined in its header, still checked end to end check_const_suggestion_external_dependency_only_declared
C2, critical (a01fbf5) a free operator declared again with its old type in a file that only the compilation database lists: "checked in every file of the compilation database" that file is judged (D17): not checked; a file that writes another operator only leaves the suggestion checked end to end check_const_suggestion_operator_declared_elsewhere
M5, minor (c38fd58) the rewrite skipped a file it could not read, and threw std::out_of_range on an edit past the end of a file the file is named, and the suggestion kept as not checked in the unit test only; no input of the analysis found that reaches it: defensive testConstSuggestionRewriteFailures
M6, minor (42c9a4d) a rewritten file that does not compile, without an error that the rewrite adds: removed not checked no input found: defensive none
I2, important (42fa604) --help and include/analysis/ConstSuggestionCheck.hpp stated the first version's rule: a suggestion is removed when the code no longer compiles or generates different code they state the three verdicts text none

On the corpora, no input reaches these fixes (Stage 3 above): they are shown by their tests only, and M6 by none.

The other minor findings are followed up apart: #250 (M1, an alias of a deduced parameter type keeps the suggestion not checked), #248 (M2, not reproduced), #251 (M3), #252 (M4), #253 (M7). #249 tracks building the Clang invocation in one function, left from Task 3.2; leaving it there does not accept the overrun.

Targeted review of these fixes (a fresh reviewer, read-only, at 42fa604; the first two points reproduced on macOS): no refutation is lost, nothing changes without the option, and the order of §5.8 and the judgement of collisions (§5.10) are kept. What it found, and what was done:

  • Important, from a01fbf5 (C2): kept for now, and described in the README. A free operator is looked up by the keyword operator, which most C++ files contain after preprocessing, through the standard headers. For a suggestion on a free operator with external linkage, every such file of the compilation database is compiled as it is and with the rewrite, and one that Clang cannot compile as it is keeps the suggestion not checked: with an unrelated z.cpp that includes <string> and has an error, "not checked with Clang: z.cpp does not compile with Clang as it is"; without the include, checked. Such a file never refutes; the cost is in compilations and verdicts. No suggestion of the corpora is on an operator. c56c99d describes it in the README, which says that it can cost far more than for a function named by an identifier; no cost was measured. It is kept for now, as decided for this PR. Narrowing it, or reducing its cost, is left out of this PR; leaving system headers out is excluded, since a file whose header, included with -isystem, declares the function must still be judged.
  • Minor, from a01fbf5: fixed in 902e12d. A function whose name starts with operator followed by $ or a non-ASCII letter (operator$x, operatoré) was looked up as the keyword operator: a file of the database that declares it with its old type was not judged, and the suggestion was reported checked (D17 missed; the same case named opx was not checked). The plan now records whether Clang's kind of the name is an operator's, and the lookup uses it. 94deb0d adds check_const_suggestion_identifier_starting_with_operator (both cases checked before 902e12d, not checked since) and check_const_suggestion_declared_in_a_system_header (an operator and an identifier declared only through a header included with -isystem: not checked; it fails when the scan leaves system headers out). It also makes check_const_suggestion_operator_declared_elsewhere assert, from the trace, the files judged and their verdicts, and name b.cpp in its docstring: when the keyword operator is not recorded, its "another operator only" case now fails, although its messages are those the former check accepted.
  • Minor texts: fixed in c56c99d. --help and the header comment state the exceptions to removal (a fatal error, or a change beyond the declarations the check plans, keeps the suggestion not checked). The README lists the C1 case and M6. The comments of the check, of rewrite() (what it detects is an edit past the end of the file) and of the facts after an error say what the code does.
  • M6's reason: changed in 6db974b. It names the first error, not an earlier warning, which the code had before the rewrite.
  • Still open. M6 has no test, nor 6db974b's change of its reason, and by reading no input reaches it. C1's boundaries with one specialization defined, or with an inheriting constructor, are not tested.

Validation

At c84b959 (2318ea2, the two fixes, their formatting and the test of a defining file that imports a module), and on a first version of the fixes:

  • Linux arm64 (Docker, Ubuntu 24.04, LLVM 20.1.8, GCC 13.3, a fresh build of c84b959): no warning; unit and ownership tests pass; the suite passes 2740/2740: 2318ea2's 2737 tests and the three checks added.
  • Linux x86_64, macos-15 and macos-latest, in CI (below): the suite passes 2740/2740 on each. On Linux x86_64, the suite ran only in CI, on the merge ref, whose analyzer code is c84b959's.
  • macOS arm64, locally (LLVM 20.1.2, AppleClang 17): at 8658cd0, an incremental build reports no warning (every file that includes the changed header recompiled), unit and ownership tests pass, and the suite passes 2739/2739. The analyzer built at c84b959 is byte for byte the same file as 8658cd0's (8f875a7 changes whitespace only), and the three new checks pass with it.
  • A first version of the fixes, not these commits: on Linux x86_64 outside CI, that version passed (Docker, Ubuntu 24.04, LLVM 20.1.8, GCC 13.3, fresh build): no warning, unit and ownership tests pass, suite 2739/2739. The first fix is the same in both versions. For the second, the review that followed added, in the code, the module implementation unit to the imports that the scan records, and the exception for functions that no other file can declare; in the test, which has 5 cases in that run and 7 at 8658cd0, the implementation-unit case, the static witness, the case of a file that imports a module and reads the rewritten header, and the resolution of a symbolic link in LLVM_DIR when the test looks for the clang++ that builds its precompiled header and header unit.

At 2318ea2 (main at f71bdac, merged in 2318ea2): on macOS arm64 (LLVM 20, AppleClang 17) and Linux arm64 (Docker, Ubuntu 24.04, LLVM 20.1.8, GCC 13), unit tests (318 expectations) and ownership tests pass, and the suite passes 2737/2737. On Linux x86_64 (Docker, Ubuntu 24.04, LLVM 20.1.8, GCC 13.3), at the start of the campaign above, the build reports no warning, unit and ownership tests pass, and the suite passes 2737/2737. These local builds are not Release builds: on macOS the tests ran on an incremental build, and a separate clean build of 2318ea2 reports no warning; the Linux arm64 build is a full one, without warning. In CI, on the merge ref e4f8e55, whose tree is 2318ea2's, the suite passes 2737/2737 on ubuntu-24.04 (Linux x86_64), macos-14 (AppleClang 15) and macos-latest (AppleClang 21). The CI's Release builds report the warnings that main has too (#258's run): one -Wunused-result in test/unit/analyzer_module_unit_tests.cpp with GCC 13, and 24 -Wpreferred-type-bitfield-enum-conversion in Clang 20's clang/Basic/LangOptions.def with AppleClang 21. The merge adds exactly main's change since 172ffd7, merged in 4f95305 (#246), and leaves the branch's own change as it was. Against the tree of c56c99d, the head before the merge (unit 313, suite 2685/2685 on macOS), what changes is #246's: its 5 unit expectations, which the suite's log repeats, and the 52 tests of its 6 fixtures under test/bound-storage, with their 6 checks that the human and JSON outputs agree. Besides, the check of the coretrace-compiler revision that a build records prints other commit ids, as on every run, since it commits, at the current date, in git repositories it creates when it runs. On macOS, each state of this batch builds without warning (incremental builds); with the code of 94deb0d, which is 42fa604's, check_const_suggestion_identifier_starting_with_operator fails on both cases.

Earlier, at 42fa604 (main at 1be6d3b, merged in 72dd688), on macOS arm64 (LLVM 20), Linux arm64 (Docker, Ubuntu 24.04, LLVM 20.1.8) and Linux x86_64 (Docker, Ubuntu 24.04, LLVM 20.1.8): built without warning; unit and ownership tests pass; the suite, as CI runs it, passes 2683/2683 on each. 4f95305 merges main at 172ffd7 (#254), which changes only .github/workflows/ci.yml and .github/workflows/test-ci-integration.yml; the analyzer's sources are those of 42fa604. At bed265d: the same on the three platforms, 2681/2681.

CI and code scanning

On c84b959 (merge ref 26db4d4, into main at 703da23; its tree differs from c84b959's only by main's changes since f71bdac: .github/workflows/ci.yml, action.yml, the LLVM 20 floor in CMakeLists.txt and scripts/build.sh, the README, CONTRIBUTING.md and the changelog): every check passes, format-check included; the release job is skipped, as outside tags.

  • The builds pass on ubuntu-24.04, macos-15 and macos-latest. On each, the suite passes 2740/2740, every case of the three new checks included. macos-14 no longer runs (ci: build on macos-15 with Xcode 16.0 before macos-14 is retired #258).
  • Code scanning: coretrace-stack-analyzer reported 83 results, coretrace-concurrency-analyzer none. The alerts open here and not on main are the seven Recursion.Detected notes listed for bed265d below (#273, #286 to #291): the fixes add or close none.

On 8658cd0 (merge ref 0cb919d, into main at 2e3c12c): the builds passed on the same three runners, 2739/2739 each, and format-check failed on the two fixes, on whitespace only; 8f875a7 formats them.

On 2318ea2 (merge ref e4f8e55, into main at f71bdac): every check passes, format-check and macos-14 included (the release job is skipped, as outside tags); macos-14 ran before the brownout of October 31. Code scanning: coretrace-stack-analyzer reported 83 results, coretrace-concurrency-analyzer none. The alerts open here and not on main are the seven Recursion.Detected notes listed for bed265d below (#273, #286 to #291), at other lines of ConstSuggestionFacts.cpp: this batch adds or closes none. The one result fewer than on 4f95305 is #224, a Recursion.Detected note in StackBufferAnalysis.cpp that #246 fixed on main too (70 results there, against 71 before).

Earlier runs:

On 4f95305 (merge ref cd0f115, into main at 172ffd7): every check passes, format-check and macos-14 included. macos-14 first failed before any step, in a scheduled brownout of the macOS 14 image (actions/runner-images#13518; the image is retired on November 2, 2026); re-run outside the brownout, it passes (Xcode 15.4): unit tests, const-suggestion-explicit-object.cpp, suite 2683/2683. Code scanning: coretrace-stack-analyzer reported 84 results, coretrace-concurrency-analyzer none. The alerts open here and not on main are the seven Recursion.Detected notes listed for bed265d below (#273, #286 to #291), at other lines of ConstSuggestionFacts.cpp: the fixes add or close none.

On bed265d (merge ref a629028, into main at 1be6d3b): every check passes, macos-14 and format-check included. Code scanning: coretrace-stack-analyzer reported 84 results, coretrace-concurrency-analyzer none. Seven alerts are open here and not on main, all Recursion.Detected notes in ConstSuggestionFacts.cpp, intended recursions over types and the AST: #273 (Inspector::TraverseDecl, below), and #286 to #291, writtenForm and its lambda (D19, the structure of a type), TraverseParmVarDecl and TraverseTypeLoc (the AST traversal), typeKey and argumentsKey (D20, mutually recursive over types and template arguments). Kept open, like the 44 notes of this rule then open on main.

CI on fe43067 (merge ref 2555947, into main at f846712), b17ff2a with the two D10 fixtures formatted: every check passes, macos-14 and format-check included (format-check had failed on b17ff2a for those fixtures alone). Code scanning: coretrace-stack-analyzer reported 78 results, coretrace-concurrency-analyzer none; no new alert. The only alert open here and not on main is still #273 (below).

Previously, CI on 4fd0dae (merge ref 14ce152, into main at f846712): every check passes. On macos-14 (AppleClang 15, Xcode 15.4), const-suggestion-explicit-object.cpp now passes in both passes, the six checks of testSymbolDemangling pass, and the suite passes 2645/2645. It failed there on 00a21b7 and 519500a, because macOS 14's demangler does not read C++23 explicit object members (#240). The fixture fails the same way on #221 (b8538c5).

Code scanning, against main at f846712: coretrace-stack-analyzer reported 78 results (71 on main), coretrace-concurrency-analyzer none. One alert is open here and not on main: #273.

The first run, on 00a21b7 (merge ref 29d24b0), had 13 new alerts, all in the new files and all reproduced locally:

…ated code

A const suggestion must compile in every analyzed file and keep the generated code.
…adata included

A const suggestion must hold in every file of the compilation database, metadata included.
…ic metadata

Recheck every file of the compilation database and compare semantic metadata.
…ly if unique

A specialization whose deduced type changes is paired only when unique, and its code compared.
Keep a suggestion whose rewrite only changes what a forwarding reference deduces.
…e code

Collapsed specializations with the same code must still be refused.
…write

A rewrite that reaches the preamble parses the file whole, and a header's warning is not new.
Name a diagnostic's file by its real path, so a header's warning is not counted as new.
Drop the precompiled preamble, its gain does not justify its invariants.
…ption

The Clang check of const suggestions runs only with its experimental option.
Check const suggestions with Clang only under --experimental-const-suggestion-check.
…moval reasons

Tests for the review of the experimental check: a static or anonymous-namespace homonym, a macro that writes several parameters, (void)f, an explicit object parameter, CRLF continuation lines, identifiers that contain const, the reason a removal gives; every const-suggestion input with and without the option; the human/JSON parity of a fixture with its own arguments.
A homonym with internal linkage in another file is another function. A macro that writes other parameters is not rewritten. (void)f is no conversion of the address. An explicit object parameter is numbered like the others. A continuation line ending in CRLF stays in its #define. An identifier that contains const keeps it. A removal names the first diagnostic the rewrite adds, error or checked warning. The compile command of each unit is resolved once per analysis.
ci.yml runs the whole run_test.py suite on Linux and macOS, with the same toolchains and configuration, the check's tests included: the dedicated job only ran them a second time.
clang-format 20, as the format check of the CI runs it. In const-suggestion-deduction-kept.cpp, the template declaration now takes one line: its expectations move from line 25 to line 24.
Code scanning alerts #268, #269 and #270 on #221: Inspector::plan's target, writtenType's context and the list lambda's node are only read. The experimental check keeps the three suggestions.
…ders

The image of v0.7.1 had LLVM's headers but not Clang's, so
ConstSuggestionCheck.cpp did not compile in it and the concurrency job
failed. v0.8.0 installs libclang-20-dev with clang-20
(CoreTrace/coretrace-concurrency-analyzer#204). Over this branch's
sources, its arm64 image exits 0, with no unit skipped and no result.

The image is pinned rather than left to the action's default, which
moves with the v0 tag.
T v1 finds the function by its decoded names and the parameter by its
debug number, plans no scope check, and removes a suggestion on any
unexplained code difference. The design gives T explicit identities, a
plan validated before the rewrite, units made of a file and its
configuration, a closed list of compared facts, established
correspondences, and three verdicts: verified, refuted, indeterminate.

Refs #217
- The checked warning groups are enabled with -W<group>, then kept as
  warnings with -Wno-error=<group>, before and after the rewrite.
- The scope invariant covers the declarations written in the sources;
  instantiated signatures are left to the correspondences.
- Heuristics have no say in a verdict.
- D16: sizeof(__PRETTY_FUNCTION__) becomes indeterminate, and the limit
  this creates is documented.

Refs #217
Count the parameters of a variadic function without its trailing entry, read the declared parameters from the debug declarations, and leave an unestablished layout without a type. Share each unit's initial compilation across entities. Record D17 and the fixture that D13 adds.
The prototype binds every case from the declaration that spilledParameter finds, with no output change on the fixtures. The subroutine type stays a fallback for an untyped declaration, only where its entries are established; the position reconciliation is dropped.
Step 9 recognizes the hidden return argument by its sret attribute instead of leaving the ... entry out of the count, which misbound optimized variadic functions. The effects list the artificial parameters that are no longer reported; Step 14 holds the review's test cases.
std::string writtenForm(const clang::ASTContext& context, clang::QualType type,
bool& deduced)
{
const clang::PrintingPolicy policy(context.getLangOpts());
const auto qualified = [&](const std::string& text)
{ return qualifiers.empty() ? text : qualifiers + " " + text; };
const auto part = [&](clang::QualType inner)
{ return writtenForm(context, inner, deduced); };
// parameter's default argument, is outside it.
bool TraverseParmVarDecl(clang::ParmVarDecl* parameter)
{
const clang::TypeSourceInfo* outer = plannedType_;

bool TraverseTypeLoc(clang::TypeLoc loc)
{
const bool planned = plannedType_ && loc == plannedType_->getTypeLoc();
// correspondences map; every other part, qualifiers included, kept as it is.
std::string typeKey(clang::QualType type, bool& nested)
{
type = type.getCanonicalType();
std::string argumentsKey(llvm::ArrayRef<clang::TemplateArgument> arguments,
bool& nested)
{
std::string out;
…d rewrites

Cases from the final review of #239. A renamed dependency that the file
only declares must keep the suggestion not checked, while one defined in
its header stays checked. A free operator declared again in a file of the
compilation database must keep it not checked, while a file with another
operator only does not. A rewrite whose file cannot be read, or no
longer has the planned text, must name the file without an exception.

rewrite() is exposed with its behavior unchanged so that the unit test
compiles; these cases fail until the next commit.
… only declared

A rewrite can change the specialization a function calls, h<int *> to
h<const int *>, when h is only declared in the file. Comparing the two
declarations proves nothing about the code another file defines for h,
and none may define the new one: the link failed after a suggestion
reported as checked. A renamed symbol that the file only declares on
both sides, other than the entity's own variants and inheriting
constructors, now keeps the suggestion not checked and names it.
…atabase

The scan of the compilation database records identifiers, and an
operator's name is not one: a file that declared operator== again with
its old type was not judged, and the suggestion was reported as checked
in every file of the database. The scan now records `operator` for an
operator, and the lookup uses the same token.
rewrite() skipped a file it could not read and threw std::out_of_range
on an edit past the end of the file. It now names the file, and the
suggestion is kept as not checked.
A rewrite that does not compile, and adds no error that explains it, is
not shown to be wrong: it is now kept as not checked instead of removed.
… and the header

Both still said that a suggestion is removed when the code no longer
compiles or generates different code. A suggestion is removed when the
rewrite adds an error or a tracked warning, or changes what Clang
resolves, and kept as not checked when it cannot be shown valid.
… declarations

Cases from the targeted review of #239's fixes. A C function named by an
identifier that starts with operator, operator$x or operatoré, and
declared again with its current type in a file of the compilation
database must keep the suggestion not checked; until the next commit it
is reported checked. A declaration made by a header included with
-isystem must keep it not checked too, for an operator and for an
identifier. The check of a free operator declared elsewhere now asserts,
from the trace, which units are judged and their verdicts.
…kind

The lookup in the compilation database took a name for an operator's when
it started with operator and went on with neither a letter, a digit nor
an underscore: operator$x and operatoré were looked up as the keyword
operator, a file that declares them again was not judged, and the
suggestion was reported checked. The plan now records whether Clang's
kind of the name is an operator's, and the lookup uses that. For a free
operator, every unit that writes the keyword is still judged, system
headers included.
…rning

When the rewritten code does not compile and the rewrite adds no error,
the reason cited the first diagnostic, which can be a tracked warning the
code already had. It now cites the first error. No input is known to
reach this path (#239 review, M6).
… its cost

--help and ConstSuggestionCheck.hpp stated removal without the steps
that stop the check first: a fatal error, or a change beyond the
declarations it plans. The README now lists a renamed symbol that the
file only declares and a failed rewrite that adds no error, and says
that an operator's suggestion compiles most C++ files of the database,
and what that costs. The comments on the check, on rewrite() and on the
facts after an error claimed more than the code does.
SizzleUnrlsd added a commit that referenced this pull request Nov 1, 2026
GitHub retires the macos-14 image on November 2, 2026, and fails its jobs
during the scheduled brownouts before that (actions/runner-images#13518):
"Build on macos-14" was cancelled before any step on #239 and #241 on
October 30.

The job moves to macos-15 with Xcode 16.0, the oldest Xcode of that
image: its AppleClang 16.0.0 is the oldest Apple compiler GitHub's
runners still provide. The step fails if the image no longer has Xcode
16.0, rather than building with another one.

Two things are then no longer tested: AppleClang 15, the compiler the
job was added for (#228), since macos-14 is the only image with Xcode
15; and the macOS 14 C++ runtime, whose demangler predates C++23
explicit object members (#240). These are losses of coverage, not a
decision to drop compatibility with either.
…conditional

In C, a conditional between two functions whose types the rewrite of one
makes differ is a constraint violation, which Clang reports as
-Wpointer-type-mismatch, a warning by default. PCRE2's
callout_enumerate_function_8 reaches pcre2_callout_enumerate that way,
and its two suggestions are reported checked (#239). The test passes f1
or f2 through ?:, with the default warnings and with -Werror
-Wno-pointer-type-mismatch: the four suggestions must be removed for
that warning; until the next commit they are reported checked. A
conditional between const blk * and blk *, which C allows, must keep its
suggestions checked.
A rewrite that makes the two callbacks of a conditional differ adds this
warning, and the suggestion was kept as checked. The group is now among
the tracked warnings: the check enables it and, under -Werror, keeps it
the warning it is by default.
Preprocessing does not read a module that a file imports. A file that
gets f's declaration from one and never names f is not judged, and the
suggestion is reported checked. The test covers a Clang header module, a
C++20 header unit, a named module and a module implementation unit: each
must leave the suggestion not checked; until the next commit, each is
reported checked. A PCH, which preprocessing reads, stays a witness: the
file is judged and declares f (D17). A refutation by another file must
still win, with the importing file in the trace, and a file that imports
a module and includes the rewritten header must still be judged. A
static function stays checked in every case.
…dule

The scan now records whether a unit imports a module: the token of an
#include that becomes an import or of a header unit, an import
declaration, or the unit's own module declaration when it implements a
module. A unit that imports one, and does not see the change otherwise,
leaves the suggestion not checked, unless the rewrite changes only the
defining file and the function is not a free function with external
linkage: no other file can declare it then. A refutation still wins.
format-check failed on 8658cd0: clang-format 20 puts the tracked
warnings one per line now that the list has six, and breaks a lambda of
the import check before its brace. Whitespace only; the analyzer built
from it is byte for byte the same.
a.cpp, alone in the compilation database, defines f and imports the
Clang header module Pre, whose header includes a.h and initializes an
int (*)(int *) with f: the rewrite of f's parameter breaks it. a.cpp's
compilation loads Pre, which the check does not rebuild with the
rewritten a.h, so Pre's declaration of f keeps its type and the
suggestion stays not checked. Without -fmodules, a.cpp reads pre.h as
text, and the rewrite is refuted at pre.h:2. No code changes with this
test: it passes as the check stands, and fails when the verdict or its
reason changes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ParameterDebugBinding unit tests expect an empty parameter to take no IR argument, which fails on Linux arm64

2 participants