[PATCH] D63376: [clang] Small improvments after Adding APValue to ConstantExpr

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

rsmith added inline comments.


================
Comment at: clang/include/clang/Sema/Sema.h:10298
+                                             bool AllowFold = true,
+                                             bool StoreResult = true);
   ExprResult VerifyIntegerConstantExpression(Expr *E, llvm::APSInt *Result,
----------------
Tyker wrote:
> rsmith wrote:
> > Do you need this new flag? No callers are passing it.
> the idea behind it is that not all integral constant expression benefit from storing the result. i have not analyzed which benefit from it and which don't. adding a FIXME would have probably be more appropriate.
It's useful to have a consistent AST representation, so I'd be inclined to at least always create a `ConstantExpr` node. Also, once we start allowing `consteval` evaluations that have side effects (for example, reflection-related actions that modify the AST), I think we'll need to always store the result to ensure we never evaluate a constant expression twice (and trigger its side-effects to occur twice).


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

https://reviews.llvm.org/D63376