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. -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20130924/12d1cdec/attachment.html>
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. - Michael Spencer -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20130924/c0a994ef/attachment.html>
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 2: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. >Yes, this comes up again and again. Chris has argued for true-on-error before as it fits with the classical pattern of integer or enum error codes, but recently has indicated a willingness to shift this stance. What makes sense to me is to give 3 possible choices: 1) bool return, true indicates success. 2) an enum with a success enumerator which == 0, and non-success error codes != 0. 3) an error class with an operator bool where true indicates the class does contain an error. It is only #3 and #1 that then are in any real contradiction at that point, and it seems easy to understand and explain due to the object being an error object. -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20130924/83d1de07/attachment.html>
On 9/24/2013 2:33 PM, Rui Ueyama 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.You have to be a bit careful here, as the return from parse is used as a exit code from lld. The Unix shell interprets 0 (false as success) as 1(true as failure). It has to be consistent throughout for this reason. As Bigcheese mentioned, if its in the Coding standard this would really help. Thanks Shankar Easwaran> > 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. >-- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by the Linux Foundation
It looks like although it's not mentioned in the coding standard, nobody is opposing to invert it for this case, so I'll do it. I agree with Michael. Returning true on success seems natural to me. However, I believe the most important thing is to maintain consistency of boolean return value at least within LLD whether it should be true or false. On Tue, Sep 24, 2013 at 1:51 PM, Shankar Easwaran <shankare at codeaurora.org>wrote:> On 9/24/2013 2:33 PM, Rui Ueyama 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. >> > You have to be a bit careful here, as the return from parse is used as a > exit code from lld. > > The Unix shell interprets 0 (false as success) as 1(true as failure). > > It has to be consistent throughout for this reason. > > As Bigcheese mentioned, if its in the Coding standard this would really > help. > > Thanks > > Shankar Easwaran > > >> 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. >> >> > > -- > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted > by the Linux Foundation > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20130924/e8df7fa2/attachment.html>