The rewrite itself stays token-shaped — it edits type SPELLINGS, which an AST
discards — but what it must not touch now comes from the AST.
The old exemption was per LINE: `int main`, `argc`, `argv`, `extern "`. Being a
transform, a missed exemption here does not over-report, it emits code that no
longer matches the API being called, so this is the rule where guessing from
substrings mattered most. And being per-line, it also disabled the rule for
anything sharing a line with one of those words.
Now a declaration with C language linkage, or one whose initialiser binds to an
entity declared outside the project, contributes a protected byte range and
keeps its spelling. That answers the case directly: a function declared in
somebody else's header taking `unsigned int` keeps `unsigned int`, and a local
initialised from strtoul keeps `unsigned long`, because of where those are
declared rather than because of what the line says.
main is protected from the start of its declaration to the opening brace of its
body, not for its whole extent. Its signature is fixed by the language; its body
is ordinary code. Three findings on this repository came out of that
distinction, all correct:
for (int i = 1; i < argc; ++i) -> for (std::int32_t i = 1; ...)
skipped before only because `argc` appeared on the line, plus Crafter::Run's own
`int argc` and return type, which are ours rather than the language's.
All three AST rules now share one interop test instead of carrying a denylist
each, and it is the same test: whose header dictates this spelling.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The rule was 125 lines: four std::regex, a hand-rolled {/} scope stack, a
cumulative paren-depth counter so a wrapped parameter list would not look like
a declaration, a 40-entry keyword denylist, and a "function-shaped line" guess
whose own comment conceded it was heuristic. Storage class came from
lineStr.contains("static "). It is now ~55 lines that ask clang what kind of
declaration each thing is and what encloses it.
The three regressions the old version carried special cases for — a call with
an inline lambda argument, a bare statement call, a one-liner method — need no
handling at all, because a call is not a declaration. Their tests pass
unchanged.
On this repository the exact version found 42 violations the heuristic had
never been able to see, all real:
- 37 members of the libclang function-pointer table added two commits ago
were PascalCase. The old varDecl regex could not match a declaration whose
type is decltype(&f), so they were silently skipped. Renamed to camelCase,
which mirrors clang_createIndex -> createIndex more closely anyway.
- Crafter.Build-Shader.cpp had a snake_case local, file_name_list, invisible
to the heuristic for the same reason (it declares a const char* array).
- Four extern "C" declarations of libc functions in tests were reported as
badly-named functions. Those are named by the C library, so C language
linkage is now an exemption — the same principled test no-char-pointer
uses, rather than another denylist entry.
New tests cover what the line-based version structurally could not reach: a
signature wrapped over several lines, `static` on its own line above the
declaration it applies to, and a member versus a local inside a method.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The rule was `\bchar\s*\*` over the text minus a substring denylist — argv,
getenv, setenv, dlerror, c_str, .data(, reinterpret_cast, extern ". Every entry
was a patch for one interop site, the list could only grow as libraries
arrived, and each entry disabled the rule for the whole LINE it appeared on.
It now walks declarations and asks the question the denylist was approximating:
whose header dictates this spelling? A declaration with C language linkage, or
one whose initialiser binds to an entity declared outside the project root, is
somebody else's API and keeps its spelling. getenv, c_str and friends are
exempt because of where they are declared, not because they are named here, so
a new external library needs no new entry.
Two bugs found while testing this, both of which had made the rule silently
pass over the entire repository:
clang_getCursorLanguage cannot be used to detect extern "C". Its default answer
is CXLanguage_C for a plain function, variable or parameter even in a C++
translation unit, so isExternC was true almost everywhere and exempted
everything. Replaced by tracking CXCursor_LinkageSpec depth during the walk,
reading the extent text to tell extern "C" from extern "C++".
Attributing any foreign reference in a subtree to the enclosing declaration was
too broad: a function that merely touched libc++ somewhere in its body would
exempt its own signature. Narrowed to initialiser contexts — a variable, field
or parameter — which is where a binding to a foreign API actually occurs.
Also: functions now carry their RESULT type rather than the whole function
type, since the parameters arrive as their own declarations and would otherwise
be reported twice. main's parameters are exempt structurally, its signature
being fixed by the language rather than chosen here.
Two sites keep an explicit lint-disable, both Crafter::Run taking main's argv
verbatim. That is two visible, reasoned suppressions in place of a denylist
that silently disabled the rule for every line mentioning one of eight tokens.
ExternalCloneDir and ExternalIncludeFlags are now exposed from :External, so a
source that includes an external dependency's headers can be parsed without
running a build to discover where they are. BuildExternal derives its own
working directory through the same function, so the two cannot drift.
Crafter.Build-Shader.cpp needed this to parse at all.
A file with no compile command — project.cpp, which LoadProject builds with its
own flags — is not a translation unit of the build graph, so AST rules skip it
the way a rule self-filters by extension. That is distinct from a file that
should have parsed and did not, which stays an error.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The transforms guard themselves against comments and raw strings before
joining or rewriting a line, because pulling text up past a `//` buries it
and reflowing a multi-line literal changes the string. Those guards were
substring probes over the raw line, so they answered the wrong question:
Line(n).contains("//") fires on // inside a string literal
Line(n).contains("R\"") fires on the characters R" inside a literal, and
MISSES a raw string opened on an earlier line
Both misfire on this repo's own sources. "MARKER" ends in R", and any string
mentioning a lint-disable directive contains //. Two wrapped call sites in
tests/Lint were being left unjoined for exactly these reasons; they join now,
and the results are in this commit.
Replaced by LineHasComment() (added with the token layer) and a new
LineHasMultiLineToken(), which reports whether any token actually covering
that line spans a line boundary — a raw string or a block comment. Backed by
a per-line bitmap derived from the token cache and invalidated with it.
The guards themselves stay: joining across a real comment or a real
multi-line literal is still unsafe, and there are tests for both. What
changes is that they now fire on comments and literals rather than on the
characters that spell them.
format-concat gets narrower as a result. It used to refuse any line
containing R" and ask for a manual fix; now only a literal that genuinely
spans lines does that, because a single-line raw string reduces to R"…" in
the stripped view, fails the plain-literal test, and travels through as an
argument with its spelling intact.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
