LLD: Returning true on success

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

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

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.

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

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.

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

Yes. And that is the case where lld was return true for errors.

Perhaps returning ErrorOr 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

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 */
}

s/ABI/API

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.

It's not very accurate to say that LLD was return true for errors. Some
parts of LLD return true for errors, while other parts returns false.

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

I don't think that ErrorOr<void> would really work well. "ec" would only be
used in this "if" condition expression because it has no value, so the
compiler would warn that "ec" is unused. We would have to write "if
(parse(xx))", which is the same code as the original but is probably a bit
more confusing.

That is why I did not say that. I said, in the cases where lld does return true for errors, it is for that reason.

Also, if I recall correctly, many of those driver cases, the code originally return an int (to match the int returned by main()). That was later changed to a bool and all the clients that were expecting non-zero to mean error remained unchanged,

Bummer about the warning…

To me the advantage of declaring a method return type like:
ErrorOr parse(…)
instead of:
bool parse(…)
Is that with bool, the client will always wonder what the bool return type means and have to read the doc (or method carefully) to figure it out. Whereas Error means the return value is used to indicate errors.

-Nick