[PATCH] D34405: [Testing/Support] Remove the const_cast in TakeExpected

Pavel Labath via llvm-commits llvm-commits at lists.llvm.org
Wed Jun 21 06:28:57 PDT 2017


Ok, I see.

Having the full class would definitely give you more flexibility in
the formatting, but it won't solve the "original expression" issue, as
this is printed in the aforementioned PredicateFormatterFromMatcher.

Fixing the "constness issue" with a const_cast is easy, but quite
ugly. I think I'd actually prefer specializing an internal gmock class
over that.

For reference, this is approximately how the specialization could look like:

template <>
class testing::internal::PredicateFormatterFromMatcher<SucceededMatcher> {
public:
  explicit PredicateFormatterFromMatcher(SucceededMatcher SM)
      : SM(std::move(SM)) {}

  AssertionResult operator()(const char *Text, llvm::Error &&Err) const {
    return operator()(Text, Err);
  }

  AssertionResult operator()(const char *Text, llvm::Error &Err) const {
    StringMatchResultListener listener;
    Matcher<llvm::Error &> M = SM;
    if (MatchPrintAndExplain(Err, M, &listener))
      return AssertionSuccess();

    std::stringstream ss;
    ss << "Value of: " << Text << "\n"
       << "Expected: ";
    M.DescribeTo(&ss);
    ss << "\n  Actual: " << listener.str();
    return AssertionFailure() << ss.str();
  }

private:
  const SucceededMatcher SM;
};


On 21 June 2017 at 14:08, Zachary Turner <zturner at google.com> wrote:
> Oh I just meant improving the error message. Chandlerc has ideas on the
> constness issue (probably involving const_cast)
>
> On Wed, Jun 21, 2017 at 3:35 AM Pavel Labath via Phabricator
> <reviews at reviews.llvm.org> wrote:
>>
>> labath added a comment.
>>
>> In https://reviews.llvm.org/D34405#785627, @zturner wrote:
>>
>> > In https://reviews.llvm.org/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.
>>
>>
>> https://reviews.llvm.org/D34405
>>
>>
>>
>


More information about the llvm-commits mailing list