[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

[Eli Friedman via Phabricator via cfe-commits]

efriedma 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:
> 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151587/new/

https://reviews.llvm.org/D151587