[lld] r240147 - [lld] Allow LLD passes to return error codes.

[David Blaikie]

On Fri, Jun 19, 2015 at 2:34 PM, David Blaikie <dblaikie at gmail.com> wrote:

>
>
> On Fri, Jun 19, 2015 at 2:24 PM, Lang Hames <lhames at gmail.com> wrote:
>
>> Hi Davide,
>>
>> This was posted for review at http://reviews.llvm.org/D10552
>> <https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_D10552&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=OZ5r9VBvBy5q2XYcqBT-4wGG0xlR7yIDM58cx6UsgjE&s=3tTxGTj-YXL0OCJprC4kLqmLi5q33gz25dAM-O--ss4&e=>
>> and reviewed by Rui, who Ok'd it. The approach was also verbally Ok'd by
>> Nick Kledzik.
>>
>> The phab review should have gone out to lld subscribers, but it doesn't
>> look like emails were sent to llvm-commits.
>>
>
> Yeah - please ensure reviews actually happen on-list. Otherwise we get
> complaints about unreviewed commits/'secret' review cabals, etc.
>
> As phab says at the top of your review "
>
>    - Always subscribe llvm-commits, cfe-commits or lldb-commits! Please
>    create a new revision, adding one of the lists."
>
>
> Basically if you don't write one of those names in the review/cc/to line
> when you create the review, you're hosed - abandon the review & start
> again. Yes, there's no warning you're about to make that mistake, only that
> message after-the-fact. It's lame.
>
> (basically once you've created the review, that initial "hey, I'm sending
> this out for review, here's the attached patch and list of changed files,
> etc" email is never sent again - so there's no way to include the mailing
> list effectively at that point - if you add them after-the-fact they'll
> just see the new message, without any context about the code review, no
> patch, etc)
>

Oh, and closing the review via 'arc' will automatically close the review.
Alternatively if you want to commit manually, including the "Differential
Revision: DXXXX" (I think that's the syntax, check other commits to be
sure) will help us follow back to the review from a commit, and also allows
Phab to auto-close the review when it picks up that commit.


>
> That's odd, but I'm still digesting Phabricator's interface, so I might
>> have missed a tag somewhere. I'll try to check next time I post a review.
>>
>> Is there any reason not to return std::error_code from passes? It seems
>> reasonable that a pass may fail. The specific case I have in mind is TLV
>> relocation processing on MachO: If the OS version min is too low we want to
>> error out and report back to the user. This kind of use seems to have been
>> anticipated: The PassManager already returns an error code, although it was
>> always default constructed (no error), and not yet checked.
>>
>> - Lang.
>>
>> On Fri, Jun 19, 2015 at 1:02 PM, Davide Italiano <davide at freebsd.org>
>> wrote:
>>
>>> On Fri, Jun 19, 2015 at 10:51 AM, Lang Hames <lhames at gmail.com> wrote:
>>> > Author: lhames
>>> > Date: Fri Jun 19 12:51:46 2015
>>> > New Revision: 240147
>>> >
>>> > URL: http://llvm.org/viewvc/llvm-project?rev=240147&view=rev
>>> <https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject-3Frev-3D240147-26view-3Drev&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=OZ5r9VBvBy5q2XYcqBT-4wGG0xlR7yIDM58cx6UsgjE&s=0qOitMInM5VJbjHR-Yy31bxNbzVT1ZG-ygCl4OMZYW4&e=>
>>> > Log:
>>> > [lld] Allow LLD passes to return error codes.
>>> >
>>>
>>> Hi Lang,
>>> this is a relatively large change. For the future it would be nice if
>>> we can discuss this before checking in (e.g. on llvm-devel).
>>> That said, do you have an use-case for this? In the past we explicitly
>>> tried to not change return type from 'void' to 'std::error_code' just
>>> in case.
>>>
>>> Thanks!
>>>
>>> --
>>> Davide
>>>
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150619/a4fc4b8c/attachment.html>