nickdesaulniers added inline comments.
================
Comment at: clang/lib/AST/ExprConstant.cpp:8360
+ // Do not constant fold an R-value.
+ if (Info.EvalMode == EvalInfo::EM_ConstantFold && !E->isLValue())
+ return false;
----------------
efriedma wrote:
> efriedma wrote:
> > nickdesaulniers wrote:
> > > efriedma wrote:
> > > > nickdesaulniers wrote:
> > > > > nickdesaulniers wrote:
> > > > > > efriedma wrote:
> > > > > > > nickdesaulniers wrote:
> > > > > > > > efriedma wrote:
> > > > > > > > > Checking isLValue() doesn't make sense; consider:
> > > > > > > > >
> > > > > > > > > ```
> > > > > > > > > struct R { mutable long x; };
> > > > > > > > > struct Z { const R &x, y; };
> > > > > > > > > Z z = { R{1}, z.x.x=10 };
> > > > > > > > > ```
> > > > > > > > >
> > > > > > > > > Maybe also want to check for EM_IgnoreSideEffects? Not sure
> > > > > > > > > what cases, if any, that would affect.
> > > > > > > > >
> > > > > > > > > We should probably check `E->getStorageDuration() ==
> > > > > > > > > SD_Static`. The cases where it's a local temporary don't hit
> > > > > > > > > the getOrCreateValue() codepath, so the evaluated value
> > > > > > > > > should be handled correctly.
> > > > > > > > >
> > > > > > > > > Checking EvalMode feels a little weird, but I guess it should
> > > > > > > > > do the right thing in the cases I can think of? I'd like a
> > > > > > > > > second opinion on this.
> > > > > > > > Changing this condition to:
> > > > > > > > ```
> > > > > > > > if (E->getStorageDuration() == SD_Static &&
> > > > > > > >
> > > > > > > > Info.EvalMode == EvalInfo::EM_ConstantFold &&
> > > > > > > >
> > > > > > > > E->isXValue())
> > > > > > > >
> > > > > > > > return false;
> > > > > > > > ```
> > > > > > > > allows all tests in tree to pass, but messes up the test case
> > > > > > > > you posted above. I'm trying to sus out what else might be
> > > > > > > > different about that test case...we should return `false` for
> > > > > > > > that, but I'm not sure what's different about that case.
> > > > > > > >
> > > > > > > > In particular, I was playing with
> > > > > > > > `E->isUsableInConstantExpressions` and
> > > > > > > > `E->getLifetimeExtendedTemporaryDecl()`, but getting your case
> > > > > > > > to work, I end up regressing
> > > > > > > > clang/test/SemaCXX/attr-require-constant-initialization.cpp...argh!!
> > > > > > > Shouldn't that just be the following?
> > > > > > >
> > > > > > > ```
> > > > > > > if (E->getStorageDuration() == SD_Static &&
> > > > > > >
> > > > > > > Info.EvalMode == EvalInfo::EM_ConstantFold)
> > > > > > >
> > > > > > > return false;
> > > > > > > ```
> > > > > > >
> > > > > > > A materialized temporary is always going to be either an LValue
> > > > > > > or an XValue, and the difference between the two isn't relevant
> > > > > > > here.
> > > > > > I wish it were that simple. Checking those two alone will produce
> > > > > > failures in the following tests:
> > > > > >
> > > > > > Failed Tests (2):
> > > > > > Clang :: CodeGenCXX/mangle-ms.cpp
> > > > > > Clang :: SemaCXX/attr-require-constant-initialization.cpp
> > > > > >
> > > > > > error: 'error' diagnostics seen but not expected:
> > > > > > File
> > > > > > /android0/llvm-project/clang/test/SemaCXX/attr-require-constant-initialization.cpp
> > > > > > Line 92: variable does not have a constant initializer
> > > > > >
> > > > > > as an example of one failure, which is basically:
> > > > > >
> > > > > > ```
> > > > > > void foo(void) {
> > > > > > __attribute__((require_constant_initialization)) static const int
> > > > > > &temp_init = 42;
> > > > > > }
> > > > > > ```
> > > > > > specifically, `-std=c++03` is the only language version that fails.
> > > > > >
> > > > > Oh, perhaps it should simply be:
> > > > >
> > > > > ```
> > > > > if (Info.EvalMode == EvalInfo::EM_ConstantFold && E->isXValue())
> > > > > return false;
> > > > > ```
> > > > >
> > > > > let me add your test case for that, too.
> > > > It looks like that case is using Expr::isConstantInitializer, which
> > > > uses EM_ConstantFold, which then blows up. No combination of the
> > > > checks you're trying will let you distinguish between that case and the
> > > > case you're trying to bail out; in both cases, the EvalMode is
> > > > EvalMode, it's an lvalue, and the storage duration is static.
> > > >
> > > > Maybe the code in question shouldn't be using isConstantInitializer at
> > > > all.
> > > >
> > > > Checking for a reference type doesn't solve anything; it just makes the
> > > > issues more complicated to reproduce.
> > > d572f82e490b alludes to `Expr::isConstantInitializer` being error
> > > prone...back in 2011...
> > >
> > > > Maybe the code in question shouldn't be using isConstantInitializer at
> > > > all.
> > >
> > > Which code? My patch doesn't introduce explicit calls to that method.
> > > Which code?
> >
> > Sema::CheckCompleteVariableDeclaration
> Maybe we can add a special case for binding SD_Static
> MaterializeTemporaryExpr to references in Expr::isConstantInitializer, so it
> doesn't try to evaluate them. (Before the call to EvaluteAsLValue.)
`Expr::isConstantInitializer` is invoked for:
```
#define ATTR __attribute__((require_constant_initialization))
void x (void) {
ATTR static const int &temp_init = 42;
}
```
it is not invoked for
```
struct R { mutable long x; };
struct Z { const R &x, y; };
Z z = { R{1}, z.x.x=10 };
```
so modifying `Expr::isConstantInitializer` rather than
`LValueExprEvaluator::VisitMaterializeTemporaryExpr` will do nothing for the
latter-case and produce incorrect results.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D151587/new/
https://reviews.llvm.org/D151587
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
