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. Mapping success/error onto a bool will always be ambiguous. Is there some better pattern? -Nick -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20130924/7197328f/attachment.html>
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 } 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? >Maybe something like: enum { Success, Failure }; or enum { Failure, Success }; -- Sean Silva> > -Nick > > > > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20130924/f98b5742/attachment.html>
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>
On Tue, Sep 24, 2013 at 6:07 PM, Sean Silva <chisophugis at gmail.com> wrote:> I think it makes a lot of sense in this case. The idea is that you > increase indentation in the "error" case.I vehemently disagree. Use the return value and type that make sense for the ABI and will be unsurprising when reading the code. Use a ! when you need to produce early-exit code flows. A note about error objects: you should always have a variable and/or type name here. This gives you the opportunity to make it perfectly unambiguous whether it was an error or success. The example I give is std::error_code: if (std::error_code ec = some_filesystem_operation(...)) { /* unambiguously have an error code, because the condition was on the error code, not the function */ } -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20130924/7f2d5c1f/attachment.html>