[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/ExprConstant.cpp:7329
+      if (Result.isLValue())
+        return Success(Result, E);
+    }
----------------
wchilders wrote:
> rsmith wrote:
> > 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`?
> > Is there some circumstance under which a glvalue ConstantExpr's APValue result is not of kind LValue?
> 
> Good question, the `Sema/switch.c` test fails if you `assert(Result.isLValue())`.
> 
> ```
> ConstantExpr 0x555562ab8760 'const int' lvalue Int: 3
> `-DeclRefExpr 0x555562ab8740 'const int' lvalue Var 0x555562ab8230 'a' 'const int'
> ```
> 
> With an attributed line no. 359: https://github.com/llvm/llvm-project/blob/4b0029995853fe37d1dc95ef96f46697c743fcad/clang/test/Sema/switch.c#L359
> 
> The offending RValue is created in SemaExpr.cpp here: https://github.com/llvm/llvm-project/blob/f87563661d6661fd4843beb8f391acd077b08dbe/clang/lib/Sema/SemaExpr.cpp#L15190
> 
> The issue stems from evaluating this as an RValue to produce an integer, then caching that RValue in an lvalue constant expression. Do you have any suggestions? Perhaps an LValue to RValue conversion should be performed on the expression if it folds correctly, so that the ConstantExpr is actually an RValue?
I think we should be inserting the lvalue-to-rvalue conversion before we even try evaluating the expression. Does this go wrong along both the C++11 and pre-C++11 codepaths? (They do rather different conversions to the expression.) In any case, we're likely missing a call to `DefaultLvalueConversion` on whichever side is failing.

Judging by the fact that `struct X { void f() { switch (0) case f: ; } };` misses the "non-static member function must be called" diagnostic only in C++98 mode, I'd imagine it's just the pre-C++11 codepath that's broken here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76438