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

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added a comment.

In D132136#3766958 <https://reviews.llvm.org/D132136#3766958>, @aaron.ballman wrote:

> 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?

Soo... I was gonna tell you that the test case is obviously not going to work right now, since record types aren't implemented at all.  //But// the test case actually works just fine. However, it also works without the lvalue-to-rvalue conversion in `EvaluteAsRValue`. Both with the old and the new interpreter.

I'm going to add an actual unit test that fails without the conversion. But it only checks a very simple case, i.e. that a `DeclRefExpr` is returned as an integer and not the lvalue.


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

https://reviews.llvm.org/D132136