cor3ntin marked 2 inline comments as done. cor3ntin added inline comments.
================ Comment at: clang/test/SemaCXX/cxx2a-consteval.cpp:612 +static_assert(is_same<long, T>::value); + +} // namespace unevaluated ---------------- rsmith wrote: > cor3ntin wrote: > > aaron.ballman wrote: > > > cor3ntin wrote: > > > > cor3ntin wrote: > > > > > aaron.ballman wrote: > > > > > > Here's an interesting test case: > > > > > > ``` > > > > > > #include <typeinfo> > > > > > > > > > > > > struct S { > > > > > > virtual void f(); > > > > > > }; > > > > > > > > > > > > struct D : S { > > > > > > void f() override; > > > > > > }; > > > > > > > > > > > > consteval S *get_s() { return nullptr; } > > > > > > > > > > > > void func() { > > > > > > (void)typeid(*get_s()); > > > > > > } > > > > > > ``` > > > > > > `typeid` still needs to evaluate its operand (due to the > > > > > > polymorphic return type of `*get_s()`), and so you should get a > > > > > > diagnostic about evaluating the side effects by calling `get_s()`. > > > > > > I think this then runs into > > > > > > https://eel.is/c++draft/expr.const#13.sentence-3 and we should > > > > > > diagnose? > > > > > Not sure! > > > > > Also, in the context of this pr, the question is also whether > > > > > `decltype(typeid(*get_s()))` should be ill-formed I think > > > > Actually, I'm reading the wording again and I really don't know anymore. > > > > `get_s()` is a constant expression, right? > > > > `*get_s()` is not, I think but is that relevant here > > > > > > > > I played with a bunch of things in the code but the more I look at it > > > > the less I'm convinced an action is needed. > > > The changes to `Sema::CheckForImmediateInvocation()` to check for an > > > unevaluated context and https://eel.is/c++draft/expr.const#13.sentence-3 > > > that say an immediate invocation shall be a constant expression are what > > > got me thinking about this code snippet in the first place. I was trying > > > to decide whether `isUnevaluatedContext()` is correct or not because with > > > `typeid`, it is potentially evaluated (so sometimes it's unevaluated). > > > > > > Interestingly, everyone comes up with a different answer: > > > https://godbolt.org/z/TqjGh1he6 and I don't (yet) know who is correct. > > @rsmith Can you enlighten us here? > > My take is that `get_s()` is a constant expression and therefore an > > immediate invocation. independently of what `*get_s()` does but I'm not > > sure if that's a correct reading. > > > > Thanks a lot! > There are a few different cases here and I don't think any compiler is > getting them all right. > > ``` > struct S { > void f(); > }; > struct T { > virtual void f(); > }; > > consteval S *null_s() { return nullptr; } > consteval S *make_s() { return new S; } > consteval T *null_t() { return nullptr; } > consteval T *make_s() { return new T; } > > void func() { > (void)typeid(*null_s()); // #1 > (void)typeid(*make_s()); // #2 > (void)typeid(*null_t()); // #3 > (void)typeid(*make_t()); // #4 > } > ``` > > Here, #3 and #4 pass an lvalue of polymorphic class type to `typeid`, so the > arguments to those `typeid`s are potentially evaluated. #1 and #2 pass an > lvalue of non-polymorphic class type, so those arguments are unevaluated > operands. So we have two immediate invocations: the `null_t()` call and the > `make_t()` call. > > Lines #1 and #2 are valid because there's no immediate invocation to check. > (Clang and GCC get this wrong and reject #2.) > Line #3 is valid because the call to `null_t()` is a constant expression. > (MSVC gets this wrong for reasons I don't understand.) > Line #4 is ill-formed because the call to `make_t()` is not a constant > expression, because it returns a heap allocation. > > The way we handle `typeid` in general is to parse its operand as an > unevaluated operand, and then later `TransformToPotentiallyEvaluated` if we > find it's a glvalue of or pointer to polymorphic class type. If you find the > above testcase isn't handled correctly, you may need to make some changes in > `TransformToPotentiallyEvaluated` to trigger the proper rebuilding. (You > might need to force it to transform `CallExpr`s that refer to `consteval` > functions even if nothing within them have changed, for example.) Thanks, this was super useful! I think clang gets everything right now - I added your scenarios as tests with the other typeid tests Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D106302/new/ https://reviews.llvm.org/D106302 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits