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

[Sam McCall via llvm-commits]

https://reviews.llvm.org/D49170 shows that approach.

I've updated clangd's https://reviews.llvm.org/D49008 to work around this
when wrapping formatv, so we're not blocked by this either way.

On Tue, Jul 10, 2018 at 7:57 PM Sam McCall <sammccall at google.com> wrote:

> 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/20180711/e45a1d2a/attachment.html>