rsmith added a comment.
There are a couple of cases where you're returning `EvalStmtResult` from a
function with a `bool` return type, that I'd like to see fixed before this
lands.
All the other comments are directed towards producing more precise behavior
when evaluating a function containing errors / value-dependent constructs. I
don't think there's any need to block fixing the crasher here on improving the
diagnostics, so I'm happy if you ignore these and commit as-is (other than
fixing the return type issue), but I think we'll want to look at these
diagnostic improvements at some point.
================
Comment at: clang/lib/AST/ExprConstant.cpp:4585
static bool EvaluateVarDecl(EvalInfo &Info, const VarDecl *VD) {
// We don't need to evaluate the initializer for a static local.
----------------
I think it would be slightly better if the dependent cases in this function
returned `noteSideEffect()` instead of `false`.
================
Comment at: clang/lib/AST/ExprConstant.cpp:4613-4625
static bool EvaluateDecl(EvalInfo &Info, const Decl *D) {
bool OK = true;
if (const VarDecl *VD = dyn_cast<VarDecl>(D))
OK &= EvaluateVarDecl(Info, VD);
if (const DecompositionDecl *DD = dyn_cast<DecompositionDecl>(D))
----------------
This function should bail out as soon as it sees an error. Otherwise, I think
```
constexpr void f() { int a = error(), b = a; }
```
... will to produce a hard error when evaluating `b`, because its initializer
appears to read from an uninitialized variable.
================
Comment at: clang/lib/AST/ExprConstant.cpp:4627
+static EvalStmtResult EvaluateDependentExpr(const Expr *E, EvalInfo &Info) {
+ assert(E->isValueDependent());
----------------
Given the number of places below that seem to need to branch on the return
value rather than returning it directly, and the use from contexts other than
statement evaluation, I think it might be better for this function to return
`bool`.
================
Comment at: clang/lib/AST/ExprConstant.cpp:4867
+ if (Inc->isValueDependent())
+ return EvaluateDependentExpr(Inc, Info);
FullExpressionRAII IncScope(Info);
----------------
If this returns `ESR_Succeeded`, we should keep going rather than returning
that value. Otherwise we'll do the wrong thing for cases like:
```
constexpr bool f() {
for (int n = 0; ; ++n, error()) { if (n == 1) return true; }
throw "bad";
}
```
... because evaluation of the `for` loop (but not the enclosing function) will
terminate at the value-dependent increment expression, so we'll think that all
evaluations of this function unconditionally throw.
================
Comment at: clang/lib/AST/ExprConstant.cpp:5026
+ if (DS->getCond()->isValueDependent())
+ return EvaluateDependentExpr(DS->getCond(), Info);
FullExpressionRAII CondScope(Info);
----------------
If the condition is value-dependent, I think we should return `ESR_Failed`
here: we don't know whether to keep going or terminate the loop, and either
answer will be wrong in some cases.
================
Comment at: clang/lib/AST/ExprConstant.cpp:5064
+ if (Inc->isValueDependent())
+ return EvaluateDependentExpr(Inc, Info);
FullExpressionRAII IncScope(Info);
----------------
As above, I think we need to keep going on `ESR_Succeeded` here.
================
Comment at: clang/lib/AST/ExprConstant.cpp:5116
+ if (FS->getCond()->isValueDependent())
+ return EvaluateDependentExpr(FS->getCond(), Info);
bool Continue = true;
----------------
We should return `ESR_Failed` unconditionally here, because we don't know which
statement to execute next.
================
Comment at: clang/lib/AST/ExprConstant.cpp:5142
+ if (FS->getInc()->isValueDependent())
+ return EvaluateDependentExpr(FS->getInc(), Info);
// Increment: ++__begin
----------------
As above, I think we need to keep going on `ESR_Succeeded` here.
================
Comment at: clang/lib/AST/ExprConstant.cpp:5896-5898
+ if ((*I)->getInit()->isValueDependent())
+ return EvaluateDependentExpr((*I)->getInit(), Info);
{
----------------
This is returning an `EvalStmtResult` from a function with a `bool` return
type. It'd also be a bit more precise to keep going after a value-dependent
expression here, so that we can diagnose non-constant constructs in the body
after a typo in the mem-initializer list.
================
Comment at: clang/lib/AST/ExprConstant.cpp:6042-6043
const Expr *Init = I->getInit();
+ if (Init->isValueDependent())
+ return EvaluateDependentExpr(Init, Info);
ThisOverrideRAII ThisOverride(*Info.CurrentCall, &SubobjectParent,
----------------
Similarly, it'd be a bit more precise to keep going if `EvaluateDependentExpr`
says it succeeded.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D84637/new/
https://reviews.llvm.org/D84637
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
