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

[Zachary Turner via llvm-commits]

Interesting idea. Regardless, lgtm for now, and we can iterate later
On Wed, Jun 21, 2017 at 6:29 AM Pavel Labath <labath at google.com> wrote:

> 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
> >>
> >>
> >>
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170621/76784128/attachment.html>