fix(lint): constexpr-constant asks clang's evaluator, not the initialiser tokens
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) <noreply@anthropic.com>
This commit is contained in:
parent
651720e494
commit
649d64ae12
5 changed files with 52 additions and 31 deletions
|
|
@ -96,6 +96,9 @@ namespace {
|
||||||
decltype(&clang_getArgType) getArgType = nullptr;
|
decltype(&clang_getArgType) getArgType = nullptr;
|
||||||
decltype(&clang_getNumArgTypes) getNumArgTypes = nullptr;
|
decltype(&clang_getNumArgTypes) getNumArgTypes = nullptr;
|
||||||
decltype(&clang_getPointeeType) getPointeeType = 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() {
|
LibClang LoadLibClang() {
|
||||||
|
|
@ -182,6 +185,9 @@ namespace {
|
||||||
bind(lib.getArgType, "clang_getArgType");
|
bind(lib.getArgType, "clang_getArgType");
|
||||||
bind(lib.getNumArgTypes, "clang_getNumArgTypes");
|
bind(lib.getNumArgTypes, "clang_getNumArgTypes");
|
||||||
bind(lib.getPointeeType, "clang_getPointeeType");
|
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.getBinaryOpcode, "clang_Cursor_getBinaryOpcode");
|
||||||
bind(lib.getUnaryOperatorKind, "clang_getCursorUnaryOperatorKind");
|
bind(lib.getUnaryOperatorKind, "clang_getCursorUnaryOperatorKind");
|
||||||
bind(lib.isConstQualifiedType, "clang_isConstQualifiedType");
|
bind(lib.isConstQualifiedType, "clang_isConstQualifiedType");
|
||||||
|
|
@ -190,6 +196,9 @@ namespace {
|
||||||
bind(lib.getArgType, "clang_getArgType");
|
bind(lib.getArgType, "clang_getArgType");
|
||||||
bind(lib.getNumArgTypes, "clang_getNumArgTypes");
|
bind(lib.getNumArgTypes, "clang_getNumArgTypes");
|
||||||
bind(lib.getPointeeType, "clang_getPointeeType");
|
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()) {
|
if (!missing.empty()) {
|
||||||
lib.handle = nullptr;
|
lib.handle = nullptr;
|
||||||
lib.error = std::format("loaded {} but it is missing {}", candidates.front(), join(missing));
|
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.isConst = lc.isConstQualifiedType(declaredType) != 0;
|
||||||
decl.isScalar = IsScalarTypeKind(declaredType.kind);
|
decl.isScalar = IsScalarTypeKind(declaredType.kind);
|
||||||
decl.isLoopVariable = walk.inLoopBinding;
|
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) {
|
if (mapped == LintDeclKind::Method) {
|
||||||
decl.isConstMethod = lc.methodIsConst(cursor) != 0;
|
decl.isConstMethod = lc.methodIsConst(cursor) != 0;
|
||||||
decl.isStaticMethod = lc.methodIsStatic(cursor) != 0;
|
decl.isStaticMethod = lc.methodIsStatic(cursor) != 0;
|
||||||
|
|
|
||||||
|
|
@ -61,7 +61,7 @@ namespace Crafter {
|
||||||
return out;
|
return out;
|
||||||
};
|
};
|
||||||
|
|
||||||
const EShMessages messages = static_cast<EShMessages>(EShMsgDefault | EShMsgVulkanRules | EShMsgSpvRules);
|
constexpr EShMessages Messages = static_cast<EShMessages>(EShMsgDefault | EShMsgVulkanRules | EShMsgSpvRules);
|
||||||
std::ifstream fileStream(path, std::ios::in | std::ios::binary);
|
std::ifstream fileStream(path, std::ios::in | std::ios::binary);
|
||||||
if (!fileStream) {
|
if (!fileStream) {
|
||||||
return fail("failed to open shader source", {});
|
return fail("failed to open shader source", {});
|
||||||
|
|
@ -86,13 +86,13 @@ namespace Crafter {
|
||||||
includeDir.pushExternalLocalDirectory(dir.generic_string());
|
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());
|
return fail("GLSL parse failed", std::string(shader.getInfoLog()) + shader.getInfoDebugLog());
|
||||||
}
|
}
|
||||||
|
|
||||||
glslang::TProgram program;
|
glslang::TProgram program;
|
||||||
program.addShader(&shader);
|
program.addShader(&shader);
|
||||||
if (!program.link(messages)) {
|
if (!program.link(Messages)) {
|
||||||
return fail("GLSL link failed", std::string(program.getInfoLog()) + program.getInfoDebugLog());
|
return fail("GLSL link failed", std::string(program.getInfoLog()) + program.getInfoDebugLog());
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -215,6 +215,10 @@ export namespace Crafter {
|
||||||
// for a declaration whose uses are all in this file, so a local rather
|
// for a declaration whose uses are all in this file, so a local rather
|
||||||
// than something with external linkage.
|
// than something with external linkage.
|
||||||
bool isMutated = false;
|
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.
|
// Method declared const. Only set for Method.
|
||||||
bool isConstMethod = false;
|
bool isConstMethod = false;
|
||||||
// Method declared static. Only set for Method.
|
// Method declared static. Only set for Method.
|
||||||
|
|
|
||||||
37
lint-rules.h
37
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,
|
// 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.
|
// which moves it into the type system instead of leaving it to the
|
||||||
// Only fires when every token of the initialiser is a literal or an
|
// optimiser. Constant-ness of the initialiser is decided by clang's own
|
||||||
// operator, so `const int A = 1 << 4;` qualifies and
|
// evaluator, so `sizeof(T)`, a fold over other constants and anything else
|
||||||
// `const int B = Compute();` does not.
|
// 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) {
|
cfg.AddAstLintRule("constexpr-constant", [](LintContext& ctx) {
|
||||||
if (!IsCppFile(ctx)) return;
|
if (!IsCppFile(ctx)) return;
|
||||||
std::span<const Crafter::LintToken> tokens = ctx.Tokens();
|
|
||||||
for (const Crafter::LintDecl& decl : ctx.Decls()) {
|
for (const Crafter::LintDecl& decl : ctx.Decls()) {
|
||||||
if (decl.kind != Crafter::LintDeclKind::Variable && decl.kind != Crafter::LintDeclKind::Field) continue;
|
if (decl.kind != Crafter::LintDeclKind::Variable && decl.kind != Crafter::LintDeclKind::Field) continue;
|
||||||
if (!decl.isConst || decl.isConstexpr || !decl.isScalar) continue;
|
if (!decl.isConst || decl.isConstexpr || !decl.isScalar) continue;
|
||||||
// A pointer's value is an address, which is rarely a constant
|
// A pointer's value is an address, which is rarely a constant
|
||||||
// expression and never an interesting one to promote.
|
// expression and never an interesting one to promote.
|
||||||
if (decl.type.contains('*')) continue;
|
if (decl.type.contains('*')) continue;
|
||||||
|
if (!decl.isConstantInitialised) continue;
|
||||||
// Walk the declaration's own tokens, starting after the '='.
|
if (decl.name.empty()) continue;
|
||||||
bool sawAssign = false;
|
ctx.Report(decl.line, std::format("'{}' has a constant initialiser — declare it constexpr", decl.name));
|
||||||
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));
|
|
||||||
}
|
}
|
||||||
});
|
});
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -478,20 +478,29 @@ int main() {
|
||||||
Check(!HasFinding(r.summary, "'each'"), "const-local: a range-for binding is not reported");
|
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
|
// constexpr-constant asks clang's constant evaluator, not the tokens of the
|
||||||
// literals and operators, so a call result is left alone.
|
// 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"
|
RuleRun r = RunRule("int Compute();\n"
|
||||||
|
"constexpr int Base = 4;\n"
|
||||||
|
"struct Big { int a, b; };\n"
|
||||||
"void F() {\n"
|
"void F() {\n"
|
||||||
" const int literal = 4;\n"
|
" const int literal = 4;\n"
|
||||||
" const int folded = 1 << 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"
|
" const int fromCall = Compute();\n"
|
||||||
" constexpr int already = 8;\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",
|
"}\n",
|
||||||
"constexpr-constant", LintMode::Report);
|
"constexpr-constant", LintMode::Report);
|
||||||
Check(HasFinding(r.summary, "'literal'"), "constexpr: a literal constant is reported");
|
Check(HasFinding(r.summary, "'literal'"), "constexpr: a literal constant is reported");
|
||||||
Check(HasFinding(r.summary, "'folded'"), "constexpr: an operator fold over literals 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, "'fromCall'"), "constexpr: a call result is not a constant expression");
|
||||||
Check(!HasFinding(r.summary, "'already'"), "constexpr: an existing constexpr is not re-reported");
|
Check(!HasFinding(r.summary, "'already'"), "constexpr: an existing constexpr is not re-reported");
|
||||||
}
|
}
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue