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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151587