llvm dev - Sep 2018 - Should functions returning bool return true or false on success?

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

Apologies for the bike-sheddy topic.

Nico
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180917/b2502481/attachment.html>

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:
> 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 :-)
>
> Apologies for the bike-sheddy topic.
>
> Nico
>
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180917/4af0134f/attachment.html>

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:cfe-dev-bounces at lists.llvm.org] On Behalf Of Philip
Reames via cfe-dev
Sent: Monday, September 17, 2018 11:15 AM
To: Nico Weber <thakis at chromium.org>; llvm-dev <llvm-dev at
lists.llvm.org>; cfe-dev <cfe-dev at lists.llvm.org>
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:
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 :-)

Apologies for the bike-sheddy topic.

Nico




_______________________________________________

LLVM Developers mailing list

llvm-dev at lists.llvm.org<mailto:llvm-dev at lists.llvm.org>

http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180917/2d9f509d/attachment.html>

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 <thakis at chromium.org>
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 :-)
>
> Apologies for the bike-sheddy topic.
>
> Nico
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180917/396a3af6/attachment.html>

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 <zturner at google.com>
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 <thakis at chromium.org>
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
>> :-)
>>
>> Apologies for the bike-sheddy topic.
>>
>> Nico
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180917/8565af7d/attachment.html>

- 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 <llvm-dev at
lists.llvm.org> wrote:
> 
> Hi,
> 
> in https://reviews.llvm.org/D52143 <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+success>
> http://llvm-cs.pcc.me.uk/?q=true+on+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
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180917/2b60475f/attachment.html>

On 17 September 2018 at 23:51, Matthias Braun via cfe-dev
<cfe-dev at lists.llvm.org> 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

On Mon, Sep 17, 2018 at 7:57 PM, Nico Weber via cfe-dev
<cfe-dev at lists.llvm.org> 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

On Tue, Sep 18, 2018 at 1:08 AM Hans Wennborg via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> On Mon, Sep 17, 2018 at 7:57 PM, Nico Weber via cfe-dev
> <cfe-dev at lists.llvm.org> 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
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180918/566bd61c/attachment.html>

On 17 September 2018 at 18:57, Nico Weber via cfe-dev <
cfe-dev at lists.llvm.org> 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 :-)
>
> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180919/cc5e370a/attachment.html>

On 9/19/2018 5:04 PM, James Courtier-Dutton via llvm-dev
wrote:
> 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.
So "analyzeBranch" returns "true" or "false" on
failure? And if it's not
obvious, does it mean that the name is wrongly chosen?

-Krzysztof