[LLVMdev] LLD: Returning true on success

[Nick Kledzik]

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.  


> 
> 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


-Nick






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