[PATCH] D49013: [Support] Allow formatv() to consume llvm::Error by-value without crashing.

[Sam McCall via llvm-commits]

Happy to put together a patch for that, deleting the template seems like
it'd give good diagnostics.

It still requires changes to formatv, so I guess this is preferred syntax
rather than complexity.
I'd like to understand why move semantics aren't explicit enough, when they
are for toString.

In any case, we can make clangd's log functions add the adapter
automatically, so the explicit/noise tradeoff can be tweaked for that use
case.

On Tue, Jul 10, 2018, 18:29 Zachary Turner <zturner at google.com> wrote:

> The more I think about it the more I feel like the explicit format adapter
> is the way to go. To prevent it from crashing if someone passes by value,
> can we just explicitly delete the instantiation of `formatProvider<Error>`?
> On Tue, Jul 10, 2018 at 9:22 AM Sam McCall <sammccall at google.com> wrote:
>
>> On Tue, Jul 10, 2018, 18:04 David Blaikie <dblaikie at gmail.com> wrote:
>>
>>> That seems plausible (op<< for Error) - but does that handle the case
>>> mentioned above "we may not actually want to produce the string (eg: if the
>>> Logger implementation decides to filter out the formatv_object_base based
>>> on level)" - is op<< still called even with this filtering? (seems like if
>>> it was, then it wouldn't avoid the stringification costs? but if it doesn't
>>> call the op<< then it'll still fail the consume semantic requirement of
>>> llvm::Error)
>>>
>> Ack, you're right. If you don't print the formatv_object, the Error isn't
>> consumed. Worse, if you print it twice, the second call sees a moved-From
>> error. Fundamentally, a non-const <<Error can't be used in a const
>> <<formatv_object.
>>
>> Thanks for pointing this out. I think D49129 is a dead end.
>> (Why can't Error just be a normal value type... so hard to reason about
>> :-( )
>>
>>
>>> On Tue, Jul 10, 2018 at 7:03 AM Sam McCall via Phabricator <
>>> reviews at reviews.llvm.org> wrote:
>>>
>>>> sammccall added a comment.
>>>>
>>>> An alternate approach in https://reviews.llvm.org/D49129: give `Error`
>>>> an overloaded `operator<<` that has the same consume semantics at
>>>> `toString` when passed rvalues.
>>>>
>>>> This makes `formatv`, `to_string`, `toString` all do basically the same
>>>> thing (which is the only sensible thing to do when passed an rvalue).
>>>>
>>>> It does require minor changes to `formatv` and `to_string` to make them
>>>> propagate the right kind of reference, but these seem reasonable.
>>>>
>>>> Overall I think I like that approach better than this one (more
>>>> consistent, Error handling lives in Error.h). WDYT?
>>>>
>>>>
>>>> Repository:
>>>>   rL LLVM
>>>>
>>>> https://reviews.llvm.org/D49013
>>>>
>>>>
>>>>
>>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180710/59b4f997/attachment.html>