[LLVMdev] LLD: Returning true on success

[Nick Kledzik]

On Sep 24, 2013, at 5:31 PM, Rui Ueyama <ruiu at google.com> wrote:
> 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.
That is why I did not say that. I said, in the cases where lld does return true for errors, it is for that reason.   

Also, if I recall correctly, many of those driver cases, the code originally return an int (to match the int returned by main()).  That was later changed to a bool and all the clients that were expecting non-zero to mean error remained unchanged, 


>> 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.
Bummer about the warning...   

To me the advantage of declaring a method return type like:
    ErrorOr<void> parse(…)
instead of:
    bool parse(…)
Is that with bool, the client will always wonder what the bool return type means and have to read the doc (or method carefully) to figure it out.  Whereas Error<void> means the return value is used to indicate errors.

-Nick





-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20130924/48a04b76/attachment.html>