wchilders marked 2 inline comments as done. wchilders added inline comments.
================ Comment at: clang/lib/AST/ExprConstant.cpp:7329 + if (Result.isLValue()) + return Success(Result, E); + } ---------------- rsmith wrote: > 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. This is a C test, the C++ language level is actually irrelevant as best I can tell. I added the missing LValue -> RValue conversion so that the APValue is generated correctly. With that things function as you'd expect, I added an assertion to the LValue base evaluator to detect this case early in development, otherwise, the logic is now shared across evaluator classes. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76438/new/ https://reviews.llvm.org/D76438 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
