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;
----------------
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.
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
