From 649d64ae1214be06580e7ece1a15d3969390d326 Mon Sep 17 00:00:00 2001 From: Jorijn van der Graaf Date: Fri, 31 Jul 2026 02:05:37 +0200 Subject: [PATCH] fix(lint): constexpr-constant asks clang's evaluator, not the initialiser tokens MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The first cut scanned the initialiser's tokens and required every one to be a literal or an operator. That is exactly the kind of approximation this work has been removing, and it was wrong in both directions: const int A = sizeof(Big); missed — `sizeof` is a keyword const int B = Base + 1; missed — `Base` is an identifier const auto C = 5_notConstexpr; would have been reported, wrongly clang_Cursor_Evaluate answers the question directly. Evaluating a variable declaration evaluates its initialiser, so anything that folds is recognised and a call result still is not. Costs nothing measurable — lint stays at ~15s. Found one real case the token version could not see: an EShMessages fold over three glslang enum constants in Crafter.Build-Shader.cpp. Promoting it to constexpr then made `naming` ask for constant naming, since a constexpr variable is a compile-time constant — so it is `Messages` now. Two rules agreeing on the same declaration is the intended behaviour. Co-Authored-By: Claude Opus 5 (1M context) --- implementations/Crafter.Build-Lint.cpp | 21 ++++++++++++++ implementations/Crafter.Build-Shader.cpp | 6 ++-- interfaces/Crafter.Build-Clang.cppm | 4 +++ lint-rules.h | 37 ++++++++---------------- tests/HouseRules/main.cpp | 15 ++++++++-- 5 files changed, 52 insertions(+), 31 deletions(-) diff --git a/implementations/Crafter.Build-Lint.cpp b/implementations/Crafter.Build-Lint.cpp index 478f303..72cf5f3 100644 --- a/implementations/Crafter.Build-Lint.cpp +++ b/implementations/Crafter.Build-Lint.cpp @@ -96,6 +96,9 @@ namespace { decltype(&clang_getArgType) getArgType = nullptr; decltype(&clang_getNumArgTypes) getNumArgTypes = nullptr; decltype(&clang_getPointeeType) getPointeeType = nullptr; + decltype(&clang_Cursor_Evaluate) evaluate = nullptr; + decltype(&clang_EvalResult_getKind) evalResultKind = nullptr; + decltype(&clang_EvalResult_dispose) disposeEvalResult = nullptr; }; LibClang LoadLibClang() { @@ -182,6 +185,9 @@ namespace { bind(lib.getArgType, "clang_getArgType"); bind(lib.getNumArgTypes, "clang_getNumArgTypes"); bind(lib.getPointeeType, "clang_getPointeeType"); + bind(lib.evaluate, "clang_Cursor_Evaluate"); + bind(lib.evalResultKind, "clang_EvalResult_getKind"); + bind(lib.disposeEvalResult, "clang_EvalResult_dispose"); bind(lib.getBinaryOpcode, "clang_Cursor_getBinaryOpcode"); bind(lib.getUnaryOperatorKind, "clang_getCursorUnaryOperatorKind"); bind(lib.isConstQualifiedType, "clang_isConstQualifiedType"); @@ -190,6 +196,9 @@ namespace { bind(lib.getArgType, "clang_getArgType"); bind(lib.getNumArgTypes, "clang_getNumArgTypes"); bind(lib.getPointeeType, "clang_getPointeeType"); + bind(lib.evaluate, "clang_Cursor_Evaluate"); + bind(lib.evalResultKind, "clang_EvalResult_getKind"); + bind(lib.disposeEvalResult, "clang_EvalResult_dispose"); if (!missing.empty()) { lib.handle = nullptr; lib.error = std::format("loaded {} but it is missing {}", candidates.front(), join(missing)); @@ -655,6 +664,18 @@ namespace { decl.isConst = lc.isConstQualifiedType(declaredType) != 0; decl.isScalar = IsScalarTypeKind(declaredType.kind); decl.isLoopVariable = walk.inLoopBinding; + // Ask clang whether the initialiser is a constant expression instead of + // inspecting its tokens. Evaluating a VarDecl evaluates its initialiser, + // so this covers sizeof, a fold over other constants, and anything else + // that folds — none of which a token scan can recognise — and it does + // not mistake a literal with a non-constexpr user-defined suffix for a + // constant. + if (mapped == LintDeclKind::Variable || mapped == LintDeclKind::Field) { + if (CXEvalResult evaluated = lc.evaluate(cursor)) { + decl.isConstantInitialised = lc.evalResultKind(evaluated) != CXEval_UnExposed; + lc.disposeEvalResult(evaluated); + } + } if (mapped == LintDeclKind::Method) { decl.isConstMethod = lc.methodIsConst(cursor) != 0; decl.isStaticMethod = lc.methodIsStatic(cursor) != 0; diff --git a/implementations/Crafter.Build-Shader.cpp b/implementations/Crafter.Build-Shader.cpp index ac1dad5..e251c50 100644 --- a/implementations/Crafter.Build-Shader.cpp +++ b/implementations/Crafter.Build-Shader.cpp @@ -61,7 +61,7 @@ namespace Crafter { return out; }; - const EShMessages messages = static_cast(EShMsgDefault | EShMsgVulkanRules | EShMsgSpvRules); + constexpr EShMessages Messages = static_cast(EShMsgDefault | EShMsgVulkanRules | EShMsgSpvRules); std::ifstream fileStream(path, std::ios::in | std::ios::binary); if (!fileStream) { return fail("failed to open shader source", {}); @@ -86,13 +86,13 @@ namespace Crafter { includeDir.pushExternalLocalDirectory(dir.generic_string()); } - if (!shader.parse(GetDefaultResources(), 100, false, messages, includeDir)) { + if (!shader.parse(GetDefaultResources(), 100, false, Messages, includeDir)) { return fail("GLSL parse failed", std::string(shader.getInfoLog()) + shader.getInfoDebugLog()); } glslang::TProgram program; program.addShader(&shader); - if (!program.link(messages)) { + if (!program.link(Messages)) { return fail("GLSL link failed", std::string(program.getInfoLog()) + program.getInfoDebugLog()); } diff --git a/interfaces/Crafter.Build-Clang.cppm b/interfaces/Crafter.Build-Clang.cppm index d69a7f3..ed18187 100644 --- a/interfaces/Crafter.Build-Clang.cppm +++ b/interfaces/Crafter.Build-Clang.cppm @@ -215,6 +215,10 @@ export namespace Crafter { // for a declaration whose uses are all in this file, so a local rather // than something with external linkage. bool isMutated = false; + // The initialiser is a constant expression, as decided by clang's own + // constant evaluator rather than by inspecting its tokens. Only set for + // Variable and Field. + bool isConstantInitialised = false; // Method declared const. Only set for Method. bool isConstMethod = false; // Method declared static. Only set for Method. diff --git a/lint-rules.h b/lint-rules.h index 2237873..b701980 100644 --- a/lint-rules.h +++ b/lint-rules.h @@ -312,39 +312,26 @@ inline void AddProjectLintRules(Crafter::Configuration& cfg) { }); // A constant whose value is already a constant expression can be constexpr, - // which puts it in the type system rather than leaving it to the optimiser. - // Only fires when every token of the initialiser is a literal or an - // operator, so `const int A = 1 << 4;` qualifies and - // `const int B = Compute();` does not. + // which moves it into the type system instead of leaving it to the + // optimiser. Constant-ness of the initialiser is decided by clang's own + // evaluator, so `sizeof(T)`, a fold over other constants and anything else + // that folds all qualify, while `Compute()` does not. + // + // Restricted to scalars on purpose: for a class type, constexpr may be + // unavailable even when the initialiser folds (a std::string constant + // cannot be constexpr at namespace scope), and the evaluator answering + // "this folds" is not the same question as "constexpr is legal here". cfg.AddAstLintRule("constexpr-constant", [](LintContext& ctx) { if (!IsCppFile(ctx)) return; - std::span tokens = ctx.Tokens(); for (const Crafter::LintDecl& decl : ctx.Decls()) { if (decl.kind != Crafter::LintDeclKind::Variable && decl.kind != Crafter::LintDeclKind::Field) continue; if (!decl.isConst || decl.isConstexpr || !decl.isScalar) continue; // A pointer's value is an address, which is rarely a constant // expression and never an interesting one to promote. if (decl.type.contains('*')) continue; - - // Walk the declaration's own tokens, starting after the '='. - bool sawAssign = false; - bool allConstant = true; - bool sawLiteral = false; - for (const Crafter::LintToken& token : tokens) { - if (token.offset < decl.nameOffset) continue; - if (token.offset >= decl.end) break; - std::string_view text = ctx.TokenText(token); - if (!sawAssign) { - if (text == "=") sawAssign = true; - continue; - } - if (token.kind == Crafter::LintTokenKind::Literal) { sawLiteral = true; continue; } - if (token.kind == Crafter::LintTokenKind::Punctuation) continue; - allConstant = false; // an identifier or keyword: not a literal fold - break; - } - if (!sawAssign || !sawLiteral || !allConstant) continue; - ctx.Report(decl.line, std::format("'{}' is a literal constant — declare it constexpr", decl.name)); + if (!decl.isConstantInitialised) continue; + if (decl.name.empty()) continue; + ctx.Report(decl.line, std::format("'{}' has a constant initialiser — declare it constexpr", decl.name)); } }); diff --git a/tests/HouseRules/main.cpp b/tests/HouseRules/main.cpp index edae0b4..e4c8058 100644 --- a/tests/HouseRules/main.cpp +++ b/tests/HouseRules/main.cpp @@ -478,20 +478,29 @@ int main() { Check(!HasFinding(r.summary, "'each'"), "const-local: a range-for binding is not reported"); } - // constexpr-constant only promotes a constant whose initialiser is made of - // literals and operators, so a call result is left alone. + // constexpr-constant asks clang's constant evaluator, not the tokens of the + // initialiser. sizeof and a fold over other constants are constant + // expressions that no token scan can recognise; a call result is not. { RuleRun r = RunRule("int Compute();\n" + "constexpr int Base = 4;\n" + "struct Big { int a, b; };\n" "void F() {\n" " const int literal = 4;\n" " const int folded = 1 << 4;\n" + " const int fromSizeof = sizeof(Big);\n" + " const int fromOtherConstant = Base + 1;\n" " const int fromCall = Compute();\n" " constexpr int already = 8;\n" - " (void)literal; (void)folded; (void)fromCall; (void)already;\n" + " (void)literal; (void)folded; (void)fromSizeof;\n" + " (void)fromOtherConstant; (void)fromCall; (void)already;\n" "}\n", "constexpr-constant", LintMode::Report); Check(HasFinding(r.summary, "'literal'"), "constexpr: a literal constant is reported"); Check(HasFinding(r.summary, "'folded'"), "constexpr: an operator fold over literals is reported"); + // Neither of these is reachable from a token scan of the initialiser. + Check(HasFinding(r.summary, "'fromSizeof'"), "constexpr: sizeof folds"); + Check(HasFinding(r.summary, "'fromOtherConstant'"), "constexpr: a fold over another constant folds"); Check(!HasFinding(r.summary, "'fromCall'"), "constexpr: a call result is not a constant expression"); Check(!HasFinding(r.summary, "'already'"), "constexpr: an existing constexpr is not re-reported"); }