hokein added inline comments.
================
Comment at: clang/lib/AST/ExprConstant.cpp:4583
if (!InitE)
return getDefaultInitValue(VD->getType(), Val);
----------------
rsmith wrote:
> The initializer might also be null because the variable is type-dependent
> (eg, `X<contains_errors> x;`), in which case assuming default-init is wrong.
> We should check for that and treat it like a value-dependent initializer.
I think you're right, added the check here, but unfortunately I didn't see
noticeable behavior changes, and didn't manage to construct one (I confirmed
that `decltype(invalid()) x;` will trigger this path).
================
Comment at: clang/lib/AST/ExprConstant.cpp:4961
}
+ if (IS->getCond()->isValueDependent())
+ return EvaluateDependentExpr(IS->getCond(), Info);
----------------
rsmith wrote:
> rsmith wrote:
> > hokein wrote:
> > > The `if` stmt (the same to `while`, `for`) is tricky -- if the condition
> > > is value-dependent, then we don't know which branch we should run into.
> > >
> > > - returning a ESR_Succeeded may lead to a bogus diagnostic ("reach the
> > > end of the function");
> > > - returning a ESR_Failed may lead to a bogus diagnostic ("never produce a
> > > constexpr");
> > >
> > > I guess what we want is to stop the evaluation, and indicate that we hit
> > > a value-dependent expression and we don't know how to evaluate it, but
> > > still treat the constexpr function as potential constexpr (but no extra
> > > diagnostics being emitted), but the current `EvalStmtResult` is not
> > > sufficient, maybe we need a new enum.
> > We should only produce the "never produce a constant expression" diagnostic
> > if we also produce a CCEDiag/FFDiag, so I think returning ESR_Failed here
> > should work.
> Should this check live in EvaluateCond instead?
> Should this check live in EvaluateCond instead?
there is a subtle difference -- doing it outside (not matter succeed or fail)
will stop any execution of the following code, while doing it in `EvaluateCond`
may continue the remaining code path if it succeeds, which may introduce bogus
diagnostics, e.g. emitting a diagnostic `constexpr evaluation hit maximum step
limit; possible infinite loop?` for the following example.
```
constexpr int foo() {
while (invalid()) {}
return 1;
}
```
Another point is that -- looks like if we just return `false` if it is a
value-dependent expr in `EvaluateCond`, it improves diagnostics (` function
never produces ` and `reached end of` diagnostics are suppressed) for the
following example
```
constexpr int test4() { // expected-error {{constexpr function never produces
a constant expression}}
if (invalid()) // expected-error {{use of undeclared identifier}}
return 1;
else
return 0;
} // expected-note {{control reached end of constexpr function}}
```
================
Comment at: clang/lib/AST/ExprConstant.cpp:5053
FullExpressionRAII IncScope(Info);
if (!EvaluateIgnoredValue(Info, FS->getInc()) || !IncScope.destroy())
return ESR_Failed;
----------------
rsmith wrote:
> Missing value dependence check here.
ah, right. Added one test.
================
Comment at: clang/lib/AST/ExprConstant.cpp:8440
} else {
+ if (SubExpr->isValueDependent())
+ return EvaluateDependentExpr(SubExpr, Info);
----------------
rsmith wrote:
> How does this happen?
I think this is a similar escaped case to CXXNewExpr as well.
================
Comment at: clang/lib/AST/ExprConstant.cpp:9145
} else if (Init) {
+ if (Init->isValueDependent())
+ return EvaluateDependentExpr(Init, Info);
----------------
rsmith wrote:
> How does this happen? Do we not propagate value-dependence from initializers
> to new-expressions?
this happens in the following case (during the constructor call), the
dependence-bits of `CXXNewExpr` are correct -- it is value-dependent,
instantiation-dependent, contains-errors.
However, we run into this codepath because the `CXXDefaultInitExpr` doesn't
have any dependence-bits being set, in fact, it is always none, I think this is
a bug, fixing in https://reviews.llvm.org/D87382.
With that fix, we don't need this check anymore (the same to other two places).
```
struct A {
int* p = new int(invalid());
}
constexpr int test2() {
A a;
return 1;
}
```
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
