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

[Nick Desaulniers via Phabricator via cfe-commits]

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:
> 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!!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151587