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
