Should functions returning bool return true or false on success?

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

Should functions returning bool return true or false on success?

Yvan Roux via cfe-dev
Hi,

in https://reviews.llvm.org/D52143 there's some uncertainty if LLVM code prefers

  if (!Function())
    // Call to function failed, deal with it

or

  if (Function())
    // Call to function failed, deal with it

(Note that this is about functions returning bool, not int.)

Folks on that review feel that returning true on success is probably what we want, but it's not documented anywhere and we do have both forms in the codebase.

True on success seems more common:

Does anyone have a pointer to previous on-list discussion on this? If not, this thread could be the place where we sort this out once and for all :-)

Apologies for the bike-sheddy topic.

Nico

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] Should functions returning bool return true or false on success?

Yvan Roux via cfe-dev

my vote: true for success, false for failure

Philip


On 09/17/2018 10:57 AM, Nico Weber via llvm-dev wrote:
Hi,

in https://reviews.llvm.org/D52143 there's some uncertainty if LLVM code prefers

  if (!Function())
    // Call to function failed, deal with it

or

  if (Function())
    // Call to function failed, deal with it

(Note that this is about functions returning bool, not int.)

Folks on that review feel that returning true on success is probably what we want, but it's not documented anywhere and we do have both forms in the codebase.

True on success seems more common:

Does anyone have a pointer to previous on-list discussion on this? If not, this thread could be the place where we sort this out once and for all :-)

Apologies for the bike-sheddy topic.

Nico


_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] Should functions returning bool return true or false on success?

Yvan Roux via cfe-dev

IMO, bool is the wrong type to return here.  We have an unambiguous alternative here specifically for this reason: llvm::Error/llvm::ErrorSuccess.

 

From: cfe-dev [mailto:[hidden email]] On Behalf Of Philip Reames via cfe-dev
Sent: Monday, September 17, 2018 11:15 AM
To: Nico Weber <[hidden email]>; llvm-dev <[hidden email]>; cfe-dev <[hidden email]>
Subject: Re: [cfe-dev] [llvm-dev] Should functions returning bool return true or false on success?

 

my vote: true for success, false for failure

Philip

 

On 09/17/2018 10:57 AM, Nico Weber via llvm-dev wrote:

Hi,

 

in https://reviews.llvm.org/D52143 there's some uncertainty if LLVM code prefers

 

  if (!Function())

    // Call to function failed, deal with it

 

or

 

  if (Function())

    // Call to function failed, deal with it

 

(Note that this is about functions returning bool, not int.)

 

Folks on that review feel that returning true on success is probably what we want, but it's not documented anywhere and we do have both forms in the codebase.


True on success seems more common:

<a href="http://llvm-cs.pcc.me.uk/?q=true&#43;on&#43;success">http://llvm-cs.pcc.me.uk/?q=true+on+success

<a href="http://llvm-cs.pcc.me.uk/?q=true&#43;on&#43;error">http://llvm-cs.pcc.me.uk/?q=true+on+error

 

Does anyone have a pointer to previous on-list discussion on this? If not, this thread could be the place where we sort this out once and for all :-)

 

Apologies for the bike-sheddy topic.

 

Nico




_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

 


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Should functions returning bool return true or false on success?

Yvan Roux via cfe-dev
In reply to this post by Yvan Roux via cfe-dev
I was one of the people that commented on that review, so you already know my opinion, but sicne it hasn't been stated in any kind of official thread and some people might not click the link I will restate it here.

I feel very strongly that true means success.  I think *every* existing occurrence of true on error should be fixed (not necessarily immediately or with high priority).  I would push back on any attempt to add a new function that returns true on failure.  

implicit integer-to-boolean conversions are bad, and so despite the fact that people often write stuff like if (!strcmp(x, y)) we should not be basing decisions about good usage on the fact that bad usage is common.  

Functions that return bools are frequently named with verbs.  It's beyond confusing to have code such as:

if (withdrawMoney(Account, Amount))
  reportAnError();

Types like llvm::Error are an exception and while it can still be confusing, some of this confusion is alleviated by the fact that you have to actually assign the return value to something or else your program will fail with an assertion.  So, for example, if the above function returned an llvm::Error, you'd assert that the error was unhandled, and you'd have to re-write it:

if (auto Err = withdrawMoney(Account, Amount))
  report(Err);


On Mon, Sep 17, 2018 at 10:57 AM Nico Weber <[hidden email]> wrote:
Hi,

in https://reviews.llvm.org/D52143 there's some uncertainty if LLVM code prefers

  if (!Function())
    // Call to function failed, deal with it

or

  if (Function())
    // Call to function failed, deal with it

(Note that this is about functions returning bool, not int.)

Folks on that review feel that returning true on success is probably what we want, but it's not documented anywhere and we do have both forms in the codebase.

True on success seems more common:

Does anyone have a pointer to previous on-list discussion on this? If not, this thread could be the place where we sort this out once and for all :-)

Apologies for the bike-sheddy topic.

Nico

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Should functions returning bool return true or false on success?

Yvan Roux via cfe-dev
Error is nice when there is more than one possible way in which something can fail, but it's overkill when all you care about is yes or no, and it also forces you to check the value, which hinders readability in some situations.

We can use Optional<T> to indicate success/failure if the return type is non-void, but when it's void the only options are bool or Error, and Error is not always ideal for the aforementioned reason.

Furthermore, the code Nico linked to is in LLVMDemangle, which does not have a dependency on libSupport, so it can't use Error or Optional anyway.

On Mon, Sep 17, 2018 at 11:18 AM Zachary Turner <[hidden email]> wrote:
I was one of the people that commented on that review, so you already know my opinion, but sicne it hasn't been stated in any kind of official thread and some people might not click the link I will restate it here.

I feel very strongly that true means success.  I think *every* existing occurrence of true on error should be fixed (not necessarily immediately or with high priority).  I would push back on any attempt to add a new function that returns true on failure.  

implicit integer-to-boolean conversions are bad, and so despite the fact that people often write stuff like if (!strcmp(x, y)) we should not be basing decisions about good usage on the fact that bad usage is common.  

Functions that return bools are frequently named with verbs.  It's beyond confusing to have code such as:

if (withdrawMoney(Account, Amount))
  reportAnError();

Types like llvm::Error are an exception and while it can still be confusing, some of this confusion is alleviated by the fact that you have to actually assign the return value to something or else your program will fail with an assertion.  So, for example, if the above function returned an llvm::Error, you'd assert that the error was unhandled, and you'd have to re-write it:

if (auto Err = withdrawMoney(Account, Amount))
  report(Err);


On Mon, Sep 17, 2018 at 10:57 AM Nico Weber <[hidden email]> wrote:
Hi,

in https://reviews.llvm.org/D52143 there's some uncertainty if LLVM code prefers

  if (!Function())
    // Call to function failed, deal with it

or

  if (Function())
    // Call to function failed, deal with it

(Note that this is about functions returning bool, not int.)

Folks on that review feel that returning true on success is probably what we want, but it's not documented anywhere and we do have both forms in the codebase.

True on success seems more common:

Does anyone have a pointer to previous on-list discussion on this? If not, this thread could be the place where we sort this out once and for all :-)

Apologies for the bike-sheddy topic.

Nico

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] Should functions returning bool return true or false on success?

Yvan Roux via cfe-dev
In reply to this post by Yvan Roux via cfe-dev
- I personally prefer return `true` on success
- I'm pretty sure we have both in LLVM
- I think the 2nd version is more common in LLVM (though this is just my gut feeling and my be biased from the areas I work on).

I'd mainly try to stay consistent with the code you find in the area of LLVM you are working on...

- Matthias

On Sep 17, 2018, at 10:57 AM, Nico Weber via llvm-dev <[hidden email]> wrote:

Hi,

in https://reviews.llvm.org/D52143 there's some uncertainty if LLVM code prefers

  if (!Function())
    // Call to function failed, deal with it

or

  if (Function())
    // Call to function failed, deal with it

(Note that this is about functions returning bool, not int.)

Folks on that review feel that returning true on success is probably what we want, but it's not documented anywhere and we do have both forms in the codebase.

True on success seems more common:

Does anyone have a pointer to previous on-list discussion on this? If not, this thread could be the place where we sort this out once and for all :-)

Apologies for the bike-sheddy topic.

Nico
_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] Should functions returning bool return true or false on success?

Yvan Roux via cfe-dev
On 17 September 2018 at 23:51, Matthias Braun via cfe-dev
<[hidden email]> wrote:
> - I personally prefer return `true` on success
> - I'm pretty sure we have both in LLVM
> - I think the 2nd version is more common in LLVM (though this is just my gut
> feeling and my be biased from the areas I work on).
>
> I'd mainly try to stay consistent with the code you find in the area of LLVM
> you are working on...

I agree that staying consistent with the surrounding code is most
important. I don't like the fact that AsmParser code often uses false
for success, but it would be substantially more confusing if
target-specific code did the opposite. It's tempting to fix this but
it would a be a disruptive change that causes some pain for
out-of-tree backends.

New code that doesn't need to match behaviour of existing functions
should IMHO use true for success and false for failure.

Best,

Alex
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Should functions returning bool return true or false on success?

Yvan Roux via cfe-dev
In reply to this post by Yvan Roux via cfe-dev
On Mon, Sep 17, 2018 at 7:57 PM, Nico Weber via cfe-dev
<[hidden email]> wrote:

> Hi,
>
> in https://reviews.llvm.org/D52143 there's some uncertainty if LLVM code
> prefers
>
>   if (!Function())
>     // Call to function failed, deal with it
>
> or
>
>   if (Function())
>     // Call to function failed, deal with it
>
> (Note that this is about functions returning bool, not int.)
>
> Folks on that review feel that returning true on success is probably what we
> want, but it's not documented anywhere and we do have both forms in the
> codebase.
>
> True on success seems more common:
> http://llvm-cs.pcc.me.uk/?q=true+on+success
> http://llvm-cs.pcc.me.uk/?q=true+on+error
>
> Does anyone have a pointer to previous on-list discussion on this? If not,
> this thread could be the place where we sort this out once and for all :-)

I don't remember on-list discussions about this, but I'd be curious to
learn about the background.

In particular, true-on-error seems pervasive in our various parsers,
both in Clang and LLVM. Is this some parser writing convention that's
also used outside LLVM, or why do we do this?

Cheers,
Hans
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] Should functions returning bool return true or false on success?

Yvan Roux via cfe-dev


On Tue, Sep 18, 2018 at 1:08 AM Hans Wennborg via llvm-dev <[hidden email]> wrote:
On Mon, Sep 17, 2018 at 7:57 PM, Nico Weber via cfe-dev
<[hidden email]> wrote:
> Hi,
>
> in https://reviews.llvm.org/D52143 there's some uncertainty if LLVM code
> prefers
>
>   if (!Function())
>     // Call to function failed, deal with it
>
> or
>
>   if (Function())
>     // Call to function failed, deal with it
>
> (Note that this is about functions returning bool, not int.)
>
> Folks on that review feel that returning true on success is probably what we
> want, but it's not documented anywhere and we do have both forms in the
> codebase.
>
> True on success seems more common:
> http://llvm-cs.pcc.me.uk/?q=true+on+success
> http://llvm-cs.pcc.me.uk/?q=true+on+error
>
> Does anyone have a pointer to previous on-list discussion on this? If not,
> this thread could be the place where we sort this out once and for all :-)

I don't remember on-list discussions about this, but I'd be curious to
learn about the background.

In particular, true-on-error seems pervasive in our various parsers,
both in Clang and LLVM. Is this some parser writing convention that's
also used outside LLVM, or why do we do this?

Nah, I believe it was an early LLVM convention (thought it was in the style guide at some point - but perhaps it was just an undocumented norm that was discussed from time to time) based on the idea that functions returning integers on failure in C APIs used zero-on-success, non-zero-on-failure - and the idea was that bool false-on-success, true-on-failure was consistent with that.

- Dave
 

Cheers,
Hans
_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Should functions returning bool return true or false on success?

Yvan Roux via cfe-dev
In reply to this post by Yvan Roux via cfe-dev
On 17 September 2018 at 18:57, Nico Weber via cfe-dev <[hidden email]> wrote:
Hi,

in https://reviews.llvm.org/D52143 there's some uncertainty if LLVM code prefers

  if (!Function())
    // Call to function failed, deal with it

or

  if (Function())
    // Call to function failed, deal with it

(Note that this is about functions returning bool, not int.)

Folks on that review feel that returning true on success is probably what we want, but it's not documented anywhere and we do have both forms in the codebase.

True on success seems more common:

Does anyone have a pointer to previous on-list discussion on this? If not, this thread could be the place where we sort this out once and for all :-)

Apologies for the bike-sheddy topic.

Nico


I would take the opinion that the code should be self documenting.
So, based on the function name, it should be obvious which it is.
e.g. Calling the function things like
isDigit()    the result is fairly obvious, it returns true if the item is a digit.

Kind Regards

James


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] Should functions returning bool return true or false on success?

Yvan Roux via cfe-dev
In reply to this post by Yvan Roux via cfe-dev
On Tue, Sep 18, 2018 at 9:33 AM David Blaikie via llvm-dev <[hidden email]> wrote:
On Tue, Sep 18, 2018 at 1:08 AM Hans Wennborg via llvm-dev <[hidden email]> wrote:
On Mon, Sep 17, 2018 at 7:57 PM, Nico Weber via cfe-dev
<[hidden email]> wrote:
> Hi,
>
> in https://reviews.llvm.org/D52143 there's some uncertainty if LLVM code
> prefers
>
>   if (!Function())
>     // Call to function failed, deal with it
>
> or
>
>   if (Function())
>     // Call to function failed, deal with it
>
> (Note that this is about functions returning bool, not int.)
>
> Folks on that review feel that returning true on success is probably what we
> want, but it's not documented anywhere and we do have both forms in the
> codebase.
>
> True on success seems more common:
> http://llvm-cs.pcc.me.uk/?q=true+on+success
> http://llvm-cs.pcc.me.uk/?q=true+on+error
>
> Does anyone have a pointer to previous on-list discussion on this? If not,
> this thread could be the place where we sort this out once and for all :-)

I don't remember on-list discussions about this, but I'd be curious to
learn about the background.

In particular, true-on-error seems pervasive in our various parsers,
both in Clang and LLVM. Is this some parser writing convention that's
also used outside LLVM, or why do we do this?

Nah, I believe it was an early LLVM convention (thought it was in the style guide at some point - but perhaps it was just an undocumented norm that was discussed from time to time) based on the idea that functions returning integers on failure in C APIs used zero-on-success, non-zero-on-failure - and the idea was that bool false-on-success, true-on-failure was consistent with that.

It was an early LLVM and Clang convention, pushed much more commonly in Clang's parser for a while.

The theory was more about "returns whether there is an error" -> "returns true on error".

That said, there were several long email discussions about this many years ago (no idea how to find them now) and there was general consensus that new APIs should probably use "true on success" instead, but that code shouldn't *mix* the two conventions as that is much more confusing.

At some point, I think it would make a lot of sense to just systematically fix all of the cases we can find of "true on error".

-Chandler
 

- Dave
 

Cheers,
Hans
_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev