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

[Tyker via Phabricator via cfe-commits]

Tyker 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,
----------------
rsmith wrote:
> 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).
I removed the StoreResult flag.
evaluating expression only once is going to be mandatory in the future but i think it is hard to reach, semantic checking often needs intermediate values. so we will need probably need to integrate semantic checking to the expression evaluator to use its intermediate value. and do so without code duplication with the non-constant context checking.


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

https://reviews.llvm.org/D63376