[LLVMdev] LLD: Returning true on success

[Rui Ueyama]

On Tue, Sep 24, 2013 at 4:44 PM, Nick Kledzik <kledzik at apple.com> wrote:

>
> On Sep 24, 2013, at 4:07 PM, Sean Silva <chisophugis at gmail.com> wrote:
>
> On Tue, Sep 24, 2013 at 4:17 PM, Nick Kledzik <kledzik at apple.com> wrote:
>
>>
>> On Sep 24, 2013, at 12:40 PM, Michael Spencer <bigcheesegs at gmail.com>
>> wrote:
>>
>> On Tue, Sep 24, 2013 at 12:33 PM, Rui Ueyama <ruiu at google.com> wrote:
>>
>>> Hi LLD developers,
>>>
>>> I'm about to make a change to invert the return value of Driver::parse()
>>> to return true on success. Currently it returns false on success.
>>>
>>> In many other functions, we return true to indicate success and false to
>>> indicate failure. The inconsistency is confusing, and fixing it should
>>> improve code readability.
>>>
>>>
>> Note that some places in LLVM use false to indicate success, not sure how
>> widespread this is. Personally I think that { if (doSomething()) } means if
>> doSomething succeeded, and thus agree with you. However, I think this is
>> something that needs to be consistent across all of LLVM and should be in
>> the coding standard.
>>
>>
>> StringRef::getAsInteger() is an example documented to return true if
>> there was an error parsing the string.
>>
>
> I think it makes a lot of sense in this case. The idea is that you
> increase indentation in the "error" case. Consider parsing 2 numbers and
> returning their sum:
>
> if (S1.getAsInteger(16, Result1)) {
> // report failure
> }
> if (S2.getAsInteger(16, Result2)) {
> // report failure
> }
> return Result1 + Result2;
>
> vs.
>
> if (S1.getAsIntegerTrueOnSuccess(16, Result1)) {
>   if (S2.getAsIntegerTrueOnSuccess(16, Result2)) {
>     return Result1 + Result2;
>   } else {
>   // report failure
>   }
> } else {
>   // report failure
> }
>
> Yes.  And that is the case where lld was return true for errors.
>

It's not very accurate to say that LLD was return true for errors. Some
parts of LLD return true for errors, while other parts returns false.

> Of course, in the latter case you would just use ! to avoid gaining
> indentation, but still, I think that if most uses of an API will require
> immediately negating the return value, then maybe the sense should be
> inverted.
>
>
>>
>> Mapping success/error onto a bool will always be ambiguous.  Is there
>> some better pattern?
>>
> Perhaps returning ErrorOr<void> instead of a bool would make it clearer.
>  In practice that would mean a ! in the if statement, which runs counter to
> your strawman that if every client uses ! that the sense should be flipped,
> e.g.:
>     if (!parse(xx))
>       // handle error
>
> or that test for error could be made obvious via:
>
>     if (error_code ec = parse(xx))
>       // handle error
>

I don't think that ErrorOr<void> would really work well. "ec" would only be
used in this "if" condition expression because it has no value, so the
compiler would warn that "ec" is unused. We would have to write "if
(parse(xx))", which is the same code as the original but is probably a bit
more confusing.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20130924/09d985bc/attachment.html>