[PATCH] D72176: Make ErrorList class default constructible and add simple push_back() method

[Lang Hames via Phabricator via llvm-commits]

lhames added a reviewer: dblaikie.
lhames added a comment.

This seems like a nice change for ErrorList.

What are you using it for though? I actually keep thinking I would like to remove ErrorList, as it has some awkward properties: it doesn't interact sensibly with isA tests (isA should really be changed to a pair of utilities: containsA, and containsOnly), and it means that Expected values have a weird asymmetry in the success and failure case: They contain exactly one success value, or (potentially) many failure values. This makes it tricky, if not impossible, to safely write a utility function that maps failures to recovery values:

  int attemptWithRecovery(Expected<int> Val) {
    if (Val)
      return *Val;
    int Fallback = 0;
    handleAllErrors(Val,
                 [&](StringError &SE) { Fallback = 1; },      // What should Fallback's value be if Val contains
                 [&](ErrorInfoBase &EIB) { Fallback = 0; });  // both a StringError and another Error?
    return Fallback;
  }

In this case the answer is "the fallback value will be based on the last error in the list", but that means you've ignored the recovery mappings for all the other errors, which probably isn't what you wanted.

In my experience, whenever I have used ErrorList it was really because I wanted to be able to report multiple errors, never because I wanted to be able to recover from them. However I think this kind of reporting is better handled by a logging idiom (like ExecutionSession::reportError).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72176/new/

https://reviews.llvm.org/D72176