[PATCH] D132136: [clang] Perform implicit lvalue-to-rvalue cast with new interpreter

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D132136#3766140 <https://reviews.llvm.org/D132136#3766140>, @tbaeder wrote:

> @aaron.ballman Can you comment on my last question? I'd like to land this patch since the others depend on it.

Sorry for the delay!

In D132136#3760738 <https://reviews.llvm.org/D132136#3760738>, @tbaeder wrote:

> In D132136#3755724 <https://reviews.llvm.org/D132136#3755724>, @aaron.ballman wrote:
>
>> The existing test coverage being wrong seems like all the more reason to add correct test coverage. LValue to RValue conversions are important to get right (lol here's a wonderful demonstration of where we didn't bother to see if we got it right that I accidentally stumbled into when trying to give you a constexpr test case: https://godbolt.org/z/bdxbers3M), especially because they're going to impact which overload gets called when picking between an `&&` and `&` overload.
>
> To be clear, can I land this patch without the unittest? I tried adding this to `EvaluateAsRValueTest.cpp` but I just run into other problems in the new interpreter :) So more unittests would definitely be good.

On the one hand, I'm not comfortable landing this without adequate testing, especially because you want to build more stuff on top of it. I think we need to make sure the foundational bits of evaluation are rock solid before building things on top of them. However, on the other hand, this is foundational enough that it's hard to test this bit without other parts of the interpreter being ready. So my preference is to try to add test coverage even if it's failing currently because other parts of the interpreter aren't ready yet. e.g., (imaginary test case, feel free to use better ones)

  template <typename Ty, typename Uy>
  struct is_same { static constexpr bool value = false; };
  
  template <typename Ty>
  struct is_same<Ty, Ty> { static constexpr bool value = true; };
  
  const volatile int i = 0;
  // Test that lvalue to rvalue conversion ends up stripping top-level qualifiers.
  // FIXME: this isn't correct yet because we don't have support for qualifier conversions yet.
  static_assert(is_same<decltype(+i), int>::value, "oh no!"); // expected-error {{static assertion failed: oh no!}}

This way, we can start getting extensive test cases written while thinking about the situations specific to lvalue to rvalue conversion (or whatever else is being implemented at the moment) instead of trying to come back and remember to write them later.

Is that reasonable, or is the interpreter in such a state where even this kind of testing is basically impossible to come up with?


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

https://reviews.llvm.org/D132136