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

Apologies for the bike-sheddy topic.

Nico

my vote: true for success, false for failure

Philip

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

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

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

  • 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

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

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

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

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

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

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

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

So "analyzeBranch" returns "true" or "false" on failure? And if it's not obvious, does it mean that the name is wrongly chosen?

-Krzysztof

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?

There are a host of issues with analyzeBranch's interface. They defy description.

Let’s start with the fact that it mutates the branch.