[PATCH] D17778: TypedError for recoverable error handling

[David Blaikie via llvm-commits]

dblaikie added inline comments.

================
Comment at: docs/ProgrammersManual.rst:419
@@ +418,3 @@
+           // Try to handle 'M'. If successful, return a success value from
+	   // the handler.
+           if (tryToHandle(M))
----------------
Indentation

================
Comment at: include/llvm/Support/Error.h:349-379
@@ +348,33 @@
+
+// Specialization for member functions of the form 'Error (const ErrT&)'.
+template <typename C, typename ErrT>
+class ErrorHandlerTraits<Error (C::*)(ErrT &)>
+    : public ErrorHandlerTraits<Error (&)(ErrT &)> {};
+
+// Specialization for member functions of the form 'Error (const ErrT&) const'.
+template <typename C, typename ErrT>
+class ErrorHandlerTraits<Error (C::*)(ErrT &) const>
+    : public ErrorHandlerTraits<Error (&)(ErrT &)> {};
+
+// Specialization for member functions of the form 'Error (const ErrT&)'.
+template <typename C, typename ErrT>
+class ErrorHandlerTraits<Error (C::*)(const ErrT &)>
+    : public ErrorHandlerTraits<Error (&)(ErrT &)> {};
+
+// Specialization for member functions of the form 'Error (const ErrT&) const'.
+template <typename C, typename ErrT>
+class ErrorHandlerTraits<Error (C::*)(const ErrT &) const>
+    : public ErrorHandlerTraits<Error (&)(ErrT &)> {};
+
+/// Specialization for member functions of the form
+/// 'Error (std::unique_ptr<ErrT>) const'.
+template <typename C, typename ErrT>
+class ErrorHandlerTraits<Error (C::*)(std::unique_ptr<ErrT>)>
+    : public ErrorHandlerTraits<Error (&)(std::unique_ptr<ErrT>)> {};
+
+/// Specialization for member functions of the form
+/// 'Error (std::unique_ptr<ErrT>) const'.
+template <typename C, typename ErrT>
+class ErrorHandlerTraits<Error (C::*)(std::unique_ptr<ErrT>) const>
+    : public ErrorHandlerTraits<Error (&)(std::unique_ptr<ErrT>)> {};
+
----------------
Where's all this get used? Didn't immediately spot any test cases related to member function error handlers.

================
Comment at: include/llvm/Support/Error.h:447-461
@@ +446,17 @@
+
+  // Helper class to log uncaught errors.
+  class LogUncaughtErrorsTo {
+  public:
+    typedef ErrorInfoBase ErrorT;
+    LogUncaughtErrorsTo(raw_ostream &OS) : OS(OS) {}
+    bool appliesTo(ErrorInfoBase &EI) const { return true; }
+    Error operator()(std::unique_ptr<ErrorInfoBase> EI) {
+      EI->log(OS);
+      OS << "\n";
+      return Error();
+    }
+
+  private:
+    raw_ostream &OS;
+  };
+
----------------
Can this not be implemented with a lambda like other handlers? (something about the "appliesTo" functionality?)

================
Comment at: include/llvm/Support/Error.h:616-617
@@ +615,4 @@
+
+    this->~Expected();
+    new (this) Expected(std::move(Other));
+  }
----------------
That feels a tad heavyweight & perhaps nicer to do with just a couple of named functions, but I'm not wedded to the notion.

================
Comment at: unittests/Support/ErrorTest.cpp:148
@@ +147,3 @@
+
+    consumeError(std::move(E));
+    consumeError(std::move(F));
----------------
Might be worth having a consumeError that takes a non-const ref (lvalue ref) so you don't have to add the std::move. Just to make the tests a bit more terse. Seems easy/handy enough.

================
Comment at: unittests/Support/ErrorTest.cpp:181
@@ +180,3 @@
+
+  // Check that named handlers of type 'Error (const Err&)' work.
+  handleAllErrors(make_error<CustomError>(42), handleCustomError);
----------------
Is it worth testing named handlers separately from function objects? Is there anything special going on in the implementation that would make these cases different?

================
Comment at: unittests/Support/ErrorTest.cpp:257-272
@@ +256,18 @@
+
+    handleAllErrors(std::move(E),
+                    [&](const CustomSubError &SE) {
+                      CustomErrorInfo2 = SE.getInfo();
+                      CustomErrorExtraInfo = SE.getExtraInfo();
+                      return Error();
+                    },
+                    [&](const CustomError &CE) {
+                      // Assert that the CustomError instance above is handled
+                      // before the
+                      // CustomSubError - joinErrors should preserve error
+                      // ordering.
+                      if (CustomErrorInfo2 != 0)
+                        abort();
+                      CustomErrorInfo1 = CE.getInfo();
+                      return Error();
+                    });
+
----------------
Probably worth bringing this sort of formatting to the attention of djasper and the clang-format folks, see if we can do anything better here

================
Comment at: unittests/Support/ErrorTest.cpp:261
@@ +260,3 @@
+                      CustomErrorExtraInfo = SE.getExtraInfo();
+                      return Error();
+                    },
----------------
why are the error handlers returning errors? (maybe I'll figure this out before I finish reviewing the patch...)

So I see from the docs this is for dynamically unhandled errors - do you have a use case in mind for that already? Could we defer adding it until we do? Might it be worth allowing void returning handlers for the common case of a handler that won't dynamically fail to handle the error?

================
Comment at: unittests/Support/ErrorTest.cpp:268-269
@@ +267,4 @@
+                      // ordering.
+                      if (CustomErrorInfo2 != 0)
+                        abort();
+                      CustomErrorInfo1 = CE.getInfo();
----------------
Presumably these two lines should be an EXPECT_ or ASSERT_ style macro, rather than an anonymous abort()?

================
Comment at: unittests/Support/ErrorTest.cpp:279-287
@@ +278,11 @@
+
+  // Test consumeError for both success and error cases.
+  {{Error E;
+  consumeError(std::move(E));
+}
+{
+  Error E = make_error<CustomError>(7);
+  consumeError(std::move(E));
+}
+}
+
----------------
what's going on witmh the indentation and brace matching here?

================
Comment at: unittests/Support/ErrorTest.cpp:320
@@ +319,3 @@
+
+// Test that we can return values from handleErrors.
+{
----------------
Test case might be easier to follow if the value returned was not itself another error (to demonstrate that it's entirely independent - maybe a string or unique_ptr<int> if you want a move-only thing, etc)

================
Comment at: unittests/Support/ErrorTest.cpp:382-385
@@ +381,6 @@
+  {
+    auto DropUncheckedExpected = []() {
+      Expected<int> A = make_error<CustomError>(42);
+    };
+    EXPECT_DEATH(DropUncheckedExpected(),
+                 "Program aborted due to an unhandled Error:")
----------------
You can just roll the contents of the lambda into the EXPECT_DEATH:

  EXPECT_DEATH({ Expected<int> A = make_error<CustomError>(42); }, ... );


http://reviews.llvm.org/D17778