Repository navigation
feat(const-param): experimental Clang check of const suggestions, version 2 - #239
Draft
SizzleUnrlsd wants to merge 159 commits into
Draft
SizzleUnrlsd wants to merge 159 commits into
SizzleUnrlsd wants to merge 159 commits into
Conversation
…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; |
This was referenced Oct 26, 2026
Open
Open
…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.
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
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.
Part of #217. Fixes #237, through #238's
d711321, carried here.The second version of the experimental Clang check of
ConstParameterNotModifiedsuggestions (--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-v1at2a5b0c1).This branch carries
d711321from #238 (#237), merged in46c036f, and6a91a79and270955efrom #241 (#240), merged in4fd0dae: 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 carries6a91a79and270955eonly becauseconst-suggestion-explicit-object.cppneeds them onmacos-14. #258, now inmain, replaces that job withmacos-15: this PR's CI, which runs on its merge ref, now builds onmacos-15, where the suite passes, with the fallback present. The order is #258, then #241, then #239: #258 is inmain, and #241 has been updated withmain(3027b09). #239 is not merged before #241; once #241 is inmain, #239 is updated withmain, 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;
2318ea2mergesmainatf71bdac(#246) and is validated.8658cd0adds the fixes of the two defects that were open at2318ea2(below),8f875a7formats them, andc84b959adds a test of a defining file that imports a module.c84b959is validated (below). One criterion is not met: the cost. In the campaigns run up to42fa604, before #246, the option added about 22 to 31 % to the self-analysis on 4 CPUs, depending on the campaign; at2318ea2, on its own sources, it adds +30.1 % (below, with the measurement reserves). The cost is not measured again after2318ea2. The plan asks for at most 10 %; this overrun is not accepted.Two defects that were open at
2318ea2are fixed at8658cd0. 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 at2318ea2.?:(test5dcbae0, fixaf1edbe).callout_enumerate_function_8'scbandcallout_data, in PCRE2'spcre2test, were reported checked. The function reachespcre2_callout_enumeratethrough a conditional,c ? f1 : f2.-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.-Werror.check_const_suggestion_conditional_between_callbackscovers the default warnings and-Werror -Wno-pointer-type-mismatch, with a conditional betweenconst T *andT *, which C allows, as its witness. In PCRE2, both suggestions are now removed for that warning (Corpora at8658cd0, below).10adf29, fix8658cd0).-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).#includethat becomes one, a C++20 header unit, an import declaration, or a module implementation unit, which imports its interface.check_const_suggestion_module_importcovers, for a functionfdeclared in a header, each of the four forms, a precompiled header as a witness (judged, D17, as before), two refutations, and astaticfunctions, which stays checked in every case. The exception's two conditions are not tested separately: in every case,fhas external linkage and is declared in a header that the rewrite changes, andshas neither.Limits on modules, kept:
2318ea2and again at8658cd0, the suggestion is not checked.-include-pch) is judged, as with a plain include, when the header that the PCH was built from is still there, unchanged.-fmodules, an#includeof 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 at2318ea2and is not checked at8658cd0.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.Measurement reserves (the campaign at
2318ea2, on one Linux x86_64 machine):--timingfigures read about 8 % high; the corpora's short passes are read in CPU time, which is real.42fa604's and 4.3 during2318ea2's, against 2.0 to 2.9 on average during the other passes, both passes without the option among them. Leaving out2318ea2's second pass, the option adds +28.9 % against the mean of the two passes without it; first pass against first pass, +27.5 %.42fa604and2318ea2differ 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.Reserves:
pointer-type-mismatchis the only group added. The other warnings by which Clang reports a C constraint violation were not reviewed:compare-distinct-pointer-types(C11 6.5.9p2; for example, a comparison between two callbacks that the rewrite of one makes differ) is not tracked, and no test covers it;6db974b;operator, which most do through the standard headers, and one of them that Clang cannot compile as it is keeps the suggestion not checked. This is kept for now, as decided for this PR, and described in the README, which says that it can cost far more than for a function named by an identifier; no cost was measured. Narrowing it is left out of this PR (below);main, replaced the job withmacos-15: this PR's CI no longer builds with AppleClang 15 or tests the macOS 14 C++ runtime. It is the only runtime the CI ran that is known to need fix(analysis): fall back to LLVM's demangler when the C++ runtime's fails #241's fallback to LLVM's demangler, whichconst-suggestion-explicit-object.cpptakes there; the runtimes ofubuntu-24.04andmacos-latestread those symbols (Symbols that the C++ runtime's demangler predates, such as C++23 explicit object members, stay mangled in the output #240), and that ofmacos-15is not measured without the fallback. That is a loss of coverage, not a decision to drop AppleClang 15 or the macOS 14 runtime.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:
checked ...line;if constexprcondition or a_Genericselection;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:
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
vttsuggested,x(C2) removedvttsuggestion (#233),xverifiedconst-suggestion-virtual-base.cppcheck_const_suggestion_operator_used_elsewherestaticfunction and an external homonym: removedcheck_const_suggestion_homonyms_across_filescheck_const_suggestion_overload_defined_elsewhereconst-suggestion-macro-shared-type.cconst-suggestion-inheriting-constructor.cpp\and spaces in a#define: the macro body rewritten, removedcheck_const_suggestion_line_endingsconst-suggestion-deduction-refused.cpp, expectation changedcheck_const_suggestion_cross_tu,check_const_suggestion_compilation_database, expectations changed;check_const_suggestion_cast_in_the_defining_filesizeof(__PRETTY_FUNCTION__): removedconst-suggestion-printed-signature.c, expectation changedOther tests added cover:
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
-Werrorthat disables a tracked warning group-Werrorand default errors as errors-Werror=<group>on a tracked groupextern "C"headerconsider passing by const reference (...); the "checked" line says passing by value is not checkedint *const p, only readconst int *p, checksconst int *const pconst int *const pconst IntPtr p)const const int *, not checkedconst int *const p, verified; a qualifier written by a macro keeps the suggestion as not checkedThe type displayed comes from the plan's candidate only, written as Clang writes it, with
struct,unionandenumwhere C needs them; a type without a name stays<anonymous type>. A unit test checks that a rewrite losing the pointer's ownconstis 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.
48f3ec1builds 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:
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.7511f50): six suggestions of this repository (parallelForx2,buildx2,isRecursiveCalleex2) 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_impland_Deque_impldestructors 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). At42fa604againstbed265d, the 98 suggestions keep their verdict and reason (final review, below).The 19 units of #215. The comparisons up to
42fa604left 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. At2318ea2(Linux x86_64), each was analyzed alone again under the same limit: without the option, 18 end in at most 7.3 s, andpcre2_match.cagain 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 generatedpcre2_chartables.cleft 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:staticin PCRE2,localin zlib) that no other unit declares. Two of them were false "checked":callout_enumerate_function_8'scbandcallout_data(defect 1, above), removed at8658cd0(below);constis 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 withoutconst, a dropped qualifier;pcre2_compile_8,pcre2_dfa_match_8andpcre2_match_8, whose declarations a macro definition ofpcre2.hwrites, 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).2318ea2and8658cd0were each built there and run once over every unit, with the option and--verbose, in the chunks and with the flags of the campaign.2318ea2gives there the 130 verdicts it gave on Linux x86_64, reasons included.8658cd0. The 4 that change are all in pcre2test:callout_enumerate_function_8'scbandcallout_data: from checked to removed, "gains a checked warning: pcre2test_inc.h:1800: pointer type mismatch".callout_enumerate_function_void_8'scbandcallout_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.c84b959differs from8658cd0's by whitespace only (on macOS, the binary is the same file), so these verdicts stand forc84b959.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 at5167998), and the unit teststestConstSuggestionNestedCorrespondences,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
bed265dagainst 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 samebed265dis the variation between campaigns. At2318ea2, in one 10 GiB campaign on one binary, on2318ea2'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,42fa604and2318ea2take 1041.9 and 1043.2 s in their first passes, their only passes without--verbosethat 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 holdspcre2_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
bed265dfound two critical defects, both reproduced end to end, one important point besides the dependency on #241 (I1, above), and seven minor ones.60a9828adds 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.3266376)h<int *>toh<const int *>;his only declared in the file, and another file instantiatesh<int *>: checked, while the rewritten code fails to linkh<int *>; withhdefined in its header, still checkedcheck_const_suggestion_external_dependency_only_declareda01fbf5)check_const_suggestion_operator_declared_elsewherec38fd58)std::out_of_rangeon an edit past the end of a filetestConstSuggestionRewriteFailures42c9a4d)42fa604)--helpandinclude/analysis/ConstSuggestionCheck.hppstated the first version's rule: a suggestion is removed when the code no longer compiles or generates different codeOn 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:a01fbf5(C2): kept for now, and described in the README. A free operator is looked up by the keywordoperator, 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 unrelatedz.cppthat 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.c56c99ddescribes 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.a01fbf5: fixed in902e12d. A function whose name starts withoperatorfollowed by$or a non-ASCII letter (operator$x,operatoré) was looked up as the keywordoperator: 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 namedopxwas not checked). The plan now records whether Clang's kind of the name is an operator's, and the lookup uses it.94deb0daddscheck_const_suggestion_identifier_starting_with_operator(both cases checked before902e12d, not checked since) andcheck_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 makescheck_const_suggestion_operator_declared_elsewhereassert, from the trace, the files judged and their verdicts, and nameb.cppin its docstring: when the keywordoperatoris not recorded, its "another operator only" case now fails, although its messages are those the former check accepted.c56c99d.--helpand 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, ofrewrite()(what it detects is an edit past the end of the file) and of the facts after an error say what the code does.6db974b. It names the first error, not an earlier warning, which the code had before the rewrite.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:c84b959): no warning; unit and ownership tests pass; the suite passes 2740/2740:2318ea2's 2737 tests and the three checks added.macos-15andmacos-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 isc84b959's.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 atc84b959is byte for byte the same file as8658cd0's (8f875a7changes whitespace only), and the three new checks pass with it.8658cd0, the implementation-unit case, thestaticwitness, the case of a file that imports a module and reads the rewritten header, and the resolution of a symbolic link inLLVM_DIRwhen the test looks for the clang++ that builds its precompiled header and header unit.At
2318ea2(mainatf71bdac, merged in2318ea2): 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 of2318ea2reports no warning; the Linux arm64 build is a full one, without warning. In CI, on the merge refe4f8e55, whose tree is2318ea2's, the suite passes 2737/2737 onubuntu-24.04(Linux x86_64),macos-14(AppleClang 15) andmacos-latest(AppleClang 21). The CI's Release builds report the warnings thatmainhas too (#258's run): one-Wunused-resultintest/unit/analyzer_module_unit_tests.cppwith GCC 13, and 24-Wpreferred-type-bitfield-enum-conversionin Clang 20'sclang/Basic/LangOptions.defwith AppleClang 21. The merge adds exactlymain's change since172ffd7, merged in4f95305(#246), and leaves the branch's own change as it was. Against the tree ofc56c99d, 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 undertest/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 of94deb0d, which is42fa604's,check_const_suggestion_identifier_starting_with_operatorfails on both cases.Earlier, at
42fa604(mainat1be6d3b, merged in72dd688), 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.4f95305mergesmainat172ffd7(#254), which changes only.github/workflows/ci.ymland.github/workflows/test-ci-integration.yml; the analyzer's sources are those of42fa604. Atbed265d: the same on the three platforms, 2681/2681.CI and code scanning
On
c84b959(merge ref26db4d4, intomainat703da23; its tree differs fromc84b959's only bymain's changes sincef71bdac:.github/workflows/ci.yml,action.yml, the LLVM 20 floor inCMakeLists.txtandscripts/build.sh, the README,CONTRIBUTING.mdand the changelog): every check passes,format-checkincluded; the release job is skipped, as outside tags.ubuntu-24.04,macos-15andmacos-latest. On each, the suite passes 2740/2740, every case of the three new checks included.macos-14no longer runs (ci: build on macos-15 with Xcode 16.0 before macos-14 is retired #258).mainare the sevenRecursion.Detectednotes listed forbed265dbelow (#273, #286 to #291): the fixes add or close none.On
8658cd0(merge ref0cb919d, intomainat2e3c12c): the builds passed on the same three runners, 2739/2739 each, andformat-checkfailed on the two fixes, on whitespace only;8f875a7formats them.On
2318ea2(merge refe4f8e55, intomainatf71bdac): every check passes,format-checkandmacos-14included (the release job is skipped, as outside tags);macos-14ran before the brownout of October 31. Code scanning: coretrace-stack-analyzer reported 83 results, coretrace-concurrency-analyzer none. The alerts open here and not onmainare the sevenRecursion.Detectednotes listed forbed265dbelow (#273, #286 to #291), at other lines ofConstSuggestionFacts.cpp: this batch adds or closes none. The one result fewer than on4f95305is #224, aRecursion.Detectednote inStackBufferAnalysis.cppthat #246 fixed onmaintoo (70 results there, against 71 before).Earlier runs:
On
4f95305(merge refcd0f115, intomainat172ffd7): every check passes,format-checkandmacos-14included.macos-14first 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 onmainare the sevenRecursion.Detectednotes listed forbed265dbelow (#273, #286 to #291), at other lines ofConstSuggestionFacts.cpp: the fixes add or close none.On
bed265d(merge refa629028, intomainat1be6d3b): every check passes,macos-14andformat-checkincluded. Code scanning: coretrace-stack-analyzer reported 84 results, coretrace-concurrency-analyzer none. Seven alerts are open here and not onmain, allRecursion.Detectednotes inConstSuggestionFacts.cpp, intended recursions over types and the AST: #273 (Inspector::TraverseDecl, below), and #286 to #291,writtenFormand its lambda (D19, the structure of a type),TraverseParmVarDeclandTraverseTypeLoc(the AST traversal),typeKeyandargumentsKey(D20, mutually recursive over types and template arguments). Kept open, like the 44 notes of this rule then open onmain.CI on
fe43067(merge ref2555947, intomainatf846712),b17ff2awith the two D10 fixtures formatted: every check passes,macos-14andformat-checkincluded (format-checkhad failed onb17ff2afor 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 onmainis still #273 (below).Previously, CI on
4fd0dae(merge ref14ce152, intomainatf846712): every check passes. Onmacos-14(AppleClang 15, Xcode 15.4),const-suggestion-explicit-object.cppnow passes in both passes, the six checks oftestSymbolDemanglingpass, and the suite passes 2645/2645. It failed there on00a21b7and519500a, 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
mainatf846712: coretrace-stack-analyzer reported 78 results (71 onmain), coretrace-concurrency-analyzer none. One alert is open here and not onmain: #273.The first run, on
00a21b7(merge ref29d24b0), had 13 new alerts, all in the new files and all reproduced locally:ConstParameterNotModified.Pointer): #278 to #285, theVisit*methods of the inspector and ofSharedTypes.519500atakes them asconst, which fixes them.ConstParameterNotModified.Reference): #277,variantSymbols(clang::CodeGenerator& generator, ...).GetMangledNameis notconst, so theconsttype does not compile: the class of ConstParameterNotModified suggests a const type that does not compile #217. Dismissed as a false positive.StackPointerEscape, critical): #274, #275, #276. Araw_string_ostreamon the stack is passed toMangleContext::mangleCXXRTTI,mangleCXXRTTINameandmangleCanonicalTypeName. In Clang 20.1.8, each writes the stream through a local mangler and keeps no reference to it: ItaniumMangle.cpp L7478-L7497, MicrosoftMangle.cpp L3970-L3983 and L4146-L4153. The rule takes the virtual call for an unresolved one (False positive: StackPointerEscape on a virtual call through the first vtable slot #183, False positive: StackPointerEscape on a virtual call through a parameter #209). Dismissed as false positives.Recursion.Detected): #273,Inspector::TraverseDecl, the recursion of the AST traversal, which is intended. Kept open, like the 44 notes of this rule then open onmain.