tbaeder added a comment. In D132136#3755724 <https://reviews.llvm.org/D132136#3755724>, @aaron.ballman wrote:
> In D132136#3753290 <https://reviews.llvm.org/D132136#3753290>, @tbaeder wrote: > >> In D132136#3751702 <https://reviews.llvm.org/D132136#3751702>, @erichkeane >> wrote: >> >>> Would be great if we had a better test here... is there anything we can do >>> to validate this is happening other than checking for that one note? >> >> `EvaluateAsRValue` is called from `Expr::EvaluateAsRValue()`, so I think it >> would be possible to write a unittest for this. But I think that would be a >> lot of effort just to test this. There is even >> `unittests/AST/EvaluateAsRValueTest.cpp` already, but it tests the wrong >> thing :( > > 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. 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
