Interesting idea.  Regardless, lgtm for now, and we can iterate later<br><div class="gmail_quote"><div dir="ltr">On Wed, Jun 21, 2017 at 6:29 AM Pavel Labath <<a href="mailto:labath@google.com">labath@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Ok, I see.<br>
<br>
Having the full class would definitely give you more flexibility in<br>
the formatting, but it won't solve the "original expression" issue, as<br>
this is printed in the aforementioned PredicateFormatterFromMatcher.<br>
<br>
Fixing the "constness issue" with a const_cast is easy, but quite<br>
ugly. I think I'd actually prefer specializing an internal gmock class<br>
over that.<br>
<br>
For reference, this is approximately how the specialization could look like:<br>
<br>
template <><br>
class testing::internal::PredicateFormatterFromMatcher<SucceededMatcher> {<br>
public:<br>
  explicit PredicateFormatterFromMatcher(SucceededMatcher SM)<br>
      : SM(std::move(SM)) {}<br>
<br>
  AssertionResult operator()(const char *Text, llvm::Error &&Err) const {<br>
    return operator()(Text, Err);<br>
  }<br>
<br>
  AssertionResult operator()(const char *Text, llvm::Error &Err) const {<br>
    StringMatchResultListener listener;<br>
    Matcher<llvm::Error &> M = SM;<br>
    if (MatchPrintAndExplain(Err, M, &listener))<br>
      return AssertionSuccess();<br>
<br>
    std::stringstream ss;<br>
    ss << "Value of: " << Text << "\n"<br>
       << "Expected: ";<br>
    M.DescribeTo(&ss);<br>
    ss << "\n  Actual: " << listener.str();<br>
    return AssertionFailure() << ss.str();<br>
  }<br>
<br>
private:<br>
  const SucceededMatcher SM;<br>
};<br>
<br>
<br>
On 21 June 2017 at 14:08, Zachary Turner <<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>> wrote:<br>
> Oh I just meant improving the error message. Chandlerc has ideas on the<br>
> constness issue (probably involving const_cast)<br>
><br>
> On Wed, Jun 21, 2017 at 3:35 AM Pavel Labath via Phabricator<br>
> <<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>> wrote:<br>
>><br>
>> labath added a comment.<br>
>><br>
>> In <a href="https://reviews.llvm.org/D34405#785627" rel="noreferrer" target="_blank">https://reviews.llvm.org/D34405#785627</a>, @zturner wrote:<br>
>><br>
>> > In <a href="https://reviews.llvm.org/D34405#785598" rel="noreferrer" target="_blank">https://reviews.llvm.org/D34405#785598</a>, @labath wrote:<br>
>> ><br>
>> > > I am not entirely thrilled by that, as that adds more bloat to the<br>
>> > > error  message<br>
>> > > ("llvm::detail::TakeExpected(std::move(original_assert_expression))<br>
>> > > failed"), but I could go with that.<br>
>> ><br>
>> ><br>
>> > I think we can fix that by using a full blown class based implementation<br>
>> > of `MatcherImpl` instead of the `MATCHER_P` macro.  But it was a bit tricky<br>
>> > so I didn't attempt it yet.<br>
>><br>
>><br>
>> This idea intrigued me, so I tried playing around with it. Unfortunately,<br>
>> I have to report back that I think this is not possible. The first thing<br>
>> that the EXPECT_THAT macro does is pass the value through the<br>
>> `testing::internal::PredicateFormatterFromMatcher` class, which takes the<br>
>> value as `const T &`. From this point on, nothing we do can help as the<br>
>> value is already unmodifyable.<br>
>><br>
>> I was able to get this working by specializing this class and adding the<br>
>> overloads for other value categories, but this is a pretty big hack, and I<br>
>> think we don't want that. In light of that, I propose to stick to the<br>
>> current implementation. I think we should optimize for error message length,<br>
>> and having an extra TakeExpected overload is a small price to pay for that.<br>
>><br>
>><br>
>> <a href="https://reviews.llvm.org/D34405" rel="noreferrer" target="_blank">https://reviews.llvm.org/D34405</a><br>
>><br>
>><br>
>><br>
><br>
</blockquote></div>