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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
