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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151587