[llvm-dev] [RFC] Error handling in LLVM libraries.

[Lang Hames via llvm-dev]

Hi James,

The complexity involved in runtime checking is minimal. In terms of source
complexity the checking code runs only ~20 lines total (it's orthogonal to
the RTTI system and utilities, which take up the bulk of the code). The
runtime overhead would be minimal in debug builds, and non-existent in
release if we turn off checking there.

Runtime checking is significantly more powerful too. Take an anti-pattern
that I've seen a few times:

for (auto &Elem : Collection) {
  if (std::error_code Err = foo(Elem))
    if (Err == recoverable_error_code) {
      // Skip items with 'recoverable' failures.
      continue;
    }
  // Do stuff with elem.
}

This is the kind of code I want to stop: The kind where we pay just enough
lip service to the error to feel like we've "handled" it, so we can get on
with what we really want to do. This code will fail at runtime with
unpredictable results if Err is anything other than 'success' or
'recoverable_error_code', but it does inspect the return type, so an
attribute won't generate any warning.



The advantage of catchTypedErrors over an if statement is that it lets you
defer errors, which we wanted to be able to do in our archive walking code:

TypedError processArchive(Archive &A) {
  TypedError Errs;

  for (auto &Obj : A) {
    if (auto Err = processObject(Obj))
      if (Err.isA<ObjectError>()) {
        // processObject failed because our object was bad. We want to
report
        // this to the user, but we also want to walk the rest of the
archive
        // to collect further diagnostics, or take other meaningful actions.
        // For now, just append 'Err' to the list of deferred errors.
        Errs = join_error(std::move(Errs), std::move(Err));
        continue;
      } else
        return join_error(std::move(Err), std::move(Errs));

    // Do more work.
  }

  return Errs;
}

and now in main, you can have:

catchTypedErrors(processArchive(A),
  handleTypedError<ObjectError>([&](std::unique_ptr<ObjectError> OE) {
    ...
  })
);

And this one handler will be able to deal with all your deferred object
errors.

For clients who know up-front that they'll never have to deal with compound
errors, the if-statement would be fine, but I think it's better not to
assume that.

I want to stress that I appreciate the distaste boilerplate, but as I
mentioned in the proposal actually catching errors is the uncommon case, so
it's probably ok if it's a little bit ugly.

Cheers,
Lang.


On Wed, Feb 3, 2016 at 7:40 AM, James Y Knight <jyknight at google.com> wrote:

> On Tue, Feb 2, 2016 at 9:23 PM, Lang Hames <lhames at gmail.com> wrote:
>
>> I see the attribute as complimentary. The runtime check provides a
>> stronger guarantee: the error cannot be dropped on any path, rather than
>> just "the result is used". The attribute can help you catch obvious
>> violations of this at compile time.
>>
>
> I agree the runtime check *can* catch something additional, it just
> doesn't feel to me like the extra complexity has been justified.
>
> Or, at least, I'd have imagined a much simpler and straightforward
> interface would be fully sufficient. E.g., instead of the all the
> catch/handle stuff, if you want to handle a particular class specially, how
> about just using an if?
>
> TypedError err = somethingThatCanFail();
> if (err) {
>   if (err.isClassOf(...)) {
>     whatever;
>   else
>     return err;
> }
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20160203/75a5d001/attachment.html>