[Testing/Support] Remove the const_cast in TakeExpected

[labath]

Summary

The const_cast in the "const" version of TakeExpected was quite
dangerous, as the function does indeed modify the apparently const
argument.

I assume the reason the const overload was added was to make the
function bind to xvalues(temporaries). That can be also achieved with
rvalue references, so I use that instead.

Using the ASSERT macros on const Expected objects will now become
illegal, but I believe that is correct, as it is not actually possible
to inspect the error stored in an Expected object without modifying it.

[zturner]

include/llvm/Testing/Support/Error.h

34

Should this be std::move(Exp)?

[zturner]

include/llvm/Testing/Support/Error.h

34

Actually wait. If we have an rvalue reference overload just delete the lvalue ref overload and put all the code in here.

[labath]

include/llvm/Testing/Support/Error.h

34

That won't work because the rvalue reference will not bind to lvalues (Expected<T> foo = bar(); ASSERT_THAT_EXPECTED(foo, Succeeded());).

This does look peculiarly recursive, but it's fine because within the body of this function "Exp" is an lvalue, so it will call the other function. std::move would actually make this recursive.

A possibly cleaner solution would be to have a TakeExpectedImpl function, taking a (lvalue) reference argument, and having both of these delegate to that. (But the only difference there would be that the non-recursivity would be more obvious, and that can also be done with a comment).

[zturner]

Right, but we have the ASSERT_THAT_EXPECTED macro, which can be re-written as ASSERT_THAT(llvm::detail::Expected(std::move(Err), Matcher))

[labath]

In D34405#785558, @zturner wrote:

Right, but we have the ASSERT_THAT_EXPECTED macro, which can be re-written as ASSERT_THAT(llvm::detail::Expected(std::move(Err), Matcher))

Ah, that would work, I haven't thought of that.

I am not entirely thrilled by that, as that adds more bloat to the error message ("llvm::detail::TakeExpected(std::move(original_assert_expression)) failed"), but I could go with that.

[zturner]

In D34405#785598, @labath wrote:

In D34405#785558, @zturner wrote:

Right, but we have the ASSERT_THAT_EXPECTED macro, which can be re-written as ASSERT_THAT(llvm::detail::Expected(std::move(Err), Matcher))

Ah, that would work, I haven't thought of that.

I am not entirely thrilled by that, as that adds more bloat to the error message ("llvm::detail::TakeExpected(std::move(original_assert_expression)) failed"), but I could go with that.

I think we can fix that by using a full blown class based implementation of MatcherImpl instead of the MATCHER_P macro. But it was a bit tricky so I didn't attempt it yet.

[labath]

In D34405#785627, @zturner wrote:

In D34405#785598, @labath wrote:

I am not entirely thrilled by that, as that adds more bloat to the error message ("llvm::detail::TakeExpected(std::move(original_assert_expression)) failed"), but I could go with that.

I think we can fix that by using a full blown class based implementation of MatcherImpl instead of the MATCHER_P macro. But it was a bit tricky so I didn't attempt it yet.

This idea intrigued me, so I tried playing around with it. Unfortunately, I have to report back that I think this is not possible. The first thing that the EXPECT_THAT macro does is pass the value through the testing::internal::PredicateFormatterFromMatcher class, which takes the value as const T &. From this point on, nothing we do can help as the value is already unmodifyable.

I was able to get this working by specializing this class and adding the overloads for other value categories, but this is a pretty big hack, and I think we don't want that. In light of that, I propose to stick to the current implementation. I think we should optimize for error message length, and having an extra TakeExpected overload is a small price to pay for that.

include/llvm/Testing/Support/Error.h

template <typename T> ExpectedHolder<T> TakeExpected(Expected<T> &Exp) {
   llvm::detail::ExpectedHolder<T> Result;
   auto &EH = static_cast<llvm::detail::ErrorHolder &>(Result);
   EH = TakeError(Exp.takeError());
   if (Result.Success)
     Result.Value = &(*Exp);
   return Result;
 }
 
-template <typename T> ExpectedHolder<T> TakeExpected(const Expected<T> &Exp) {
-  return TakeExpected(const_cast<Expected<T> &>(Exp));
+template <typename T> ExpectedHolder<T> TakeExpected(Expected<T> &&Exp) {
+  return TakeExpected(Exp);
 }
 } // namespace detail
 
 #define EXPECT_THAT_ERROR(Err, Matcher)                                        \
   EXPECT_THAT(llvm::detail::TakeError(Err), Matcher)
 #define ASSERT_THAT_ERROR(Err, Matcher)                                        \
   ASSERT_THAT(llvm::detail::TakeError(Err), Matcher)