[PATCH] D76438: ConstantExpr cached APValues if present for constant evaluation

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: clang/lib/AST/Expr.cpp:2875
 
-  else if (auto *CE = dyn_cast<ConstantExpr>(E))
-    return CE->getSubExpr();
-
   return E;
 }
----------------
wchilders wrote:
> `IgnoreParensSingleStep` for some reason has been unwrapping `ConstantExpr`s. This results in the constant evaluator, removing the ConstantExpr, and reevaluating the expression. There are no observed downsides to removing this condition, in the test suite, however, it's strange enough to note.
We sadly don't really have any good, rigorous definitions of what the various `Ignore*` functions do. But the general idea of `IgnoreParens` is to ignore syntax with no semantic effect, and `ConstantExpr` doesn't describe syntax, so it probably shouldn't be stepped over here. (It is ignored by `IgnoreImplicit`, which is generally -- mostly -- trying to ignore semantics with no syntactic representation, which seems appropriate. It might make sense for `IgnoreImpCasts` to not step over `FullExpr`s, but I expect that change would break some testcases.)


================
Comment at: clang/lib/AST/ExprConstant.cpp:7329
+      if (Result.isLValue())
+        return Success(Result, E);
+    }
----------------
wchilders wrote:
> This doesn't seem to be the right answer, and `ConstantExpr`s don't have `LValue` `APValue`s, at least, not that are reaching this case. We had a previous implementation that also, kind of punted on this issue with an override in `TemporaryExprEvaluator`: https://gitlab.com/lock3/clang/-/blob/9fbaeea06fc567ac472264bec2a72661a1e06c73/clang/lib/AST/ExprConstant.cpp#L9753
The base class version seems appropriate to me, even for this case. We eventually want to use `ConstantExpr` to store the evaluated initializer value of a `constexpr` variable (replacing the existing special-case caching on `VarDecl`) and the like, not only for immediate invocations, and once we start doing that for reference variables we'll have glvalue `ConstantExpr`s.

Is there some circumstance under which a glvalue `ConstantExpr`'s `APValue` result is not of kind `LValue`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76438