[PATCH] D17778: TypedError for recoverable error handling

[David Blaikie via llvm-commits]

dblaikie added a comment.

Looking pretty good. Tried applying the patch - death tests that check for the specific text of the assertion are not portable as you've written them, because different platforms describe the assertion differently ("Assertion failed: "x"" versus "Assertion `x` failed"). Maybe just look for the text message you've passed to the assertion, rather than any flavor text around it. Though even that's not /whole/y reliable, of course (up to the implementation what it puts in that message) - but may work on the platforms we care about.

I think the ErrorHandlerTraits + test cases might be able to be simplified/reduced by implementing some more general and orthogonal constructs. If we had a trait for "arguments to a callable" (as is described here: http://stackoverflow.com/questions/21465394/accept-any-kind-of-callable-and-also-know-argument-type ) and then just used std::result_of, we could probably get away from this code having to worry about members V non-members, etc. Might be able to do the unique_ptr versus ref variants just with overloading & decltype to get the return type... it's a bit hand wavy, I realize, but happy to work through it in more detail if you like.


================
Comment at: include/llvm/Support/Error.h:492
@@ +491,3 @@
+                    OS << "\n";
+                    return Error::success();
+                  });
----------------
Drop this?

================
Comment at: include/llvm/Support/Error.h:505
@@ +504,3 @@
+  handleAllErrors(std::move(Err),
+                  [](const ErrorInfoBase &) { return Error::success(); });
+}
----------------
Drop the return here?

================
Comment at: unittests/Support/ErrorTest.cpp:290
@@ +289,3 @@
+                      CustomErrorExtraInfo = SE.getExtraInfo();
+                      return Error();
+                    },
----------------
Drop this?

================
Comment at: unittests/Support/ErrorTest.cpp:301
@@ +300,3 @@
+                      CustomErrorInfo1 = CE.getInfo();
+                      return Error();
+                    });
----------------
And this?


http://reviews.llvm.org/D17778