[PATCH] D17778: TypedError for recoverable error handling
Lang Hames via llvm-commits
llvm-commits at lists.llvm.org
Mon Mar 14 17:29:47 PDT 2016
lhames marked 2 inline comments as done.
================
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>)> {};
+
----------------
dblaikie wrote:
> Where's all this get used? Didn't immediately spot any test cases related to member function error handlers.
Lambdas. :)
The six empty specializations are for (in order)
[](ErrT&) mutable { ... }
[](ErrT&) { ... }
[](const ErrT&) mutable { ... }
[](const ErrT&) { ... }
[](std::unique_ptr<ErrT>) mutable { ... }
[](std::unique_ptr<ErrT>) { ... }
I think 90% of lambdas will be of the form 'const ErrT&', and another 9% will be 'std::unique_ptr<ErrT>' (which is useful when you want to modify and "re-throw" an error).
The other combinations - mutable versions of the above, plus the non-const reference versions, were just signatures that I felt would be useful for debugging handlers or error types by adding counters.
================
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;
+ };
+
----------------
dblaikie wrote:
> Can this not be implemented with a lambda like other handlers? (something about the "appliesTo" functionality?)
Indeed it can - this is a hangover from the earlier design. I'll remove it.
================
Comment at: include/llvm/Support/Error.h:616-617
@@ +615,4 @@
+
+ this->~Expected();
+ new (this) Expected(std::move(Other));
+ }
----------------
dblaikie wrote:
> That feels a tad heavyweight & perhaps nicer to do with just a couple of named functions, but I'm not wedded to the notion.
This was just yanked over from ErrorOr. I'm not wedded to it either, but it's battle-tested, Since getting the corner cases right for this may be tricky I'd rather change it post-commit once this code has been hammered on a bit.
================
Comment at: unittests/Support/ErrorTest.cpp:148
@@ +147,3 @@
+
+ consumeError(std::move(E));
+ consumeError(std::move(F));
----------------
dblaikie wrote:
> 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.
Sounds reasonable. I'll add that.
================
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);
----------------
dblaikie wrote:
> 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?
Named handlers are function pointers, whereas lambdas are member functions. These tests are exercising different ErrorHandlerTraits specializations.
================
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();
+ });
+
----------------
dblaikie wrote:
> 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
This is actually clang-format output, and I think it gets it right here.
Very short lambdas may get put on the same line, which looks odd - I could suggest that they specialize that.
================
Comment at: unittests/Support/ErrorTest.cpp:261
@@ +260,3 @@
+ CustomErrorExtraInfo = SE.getExtraInfo();
+ return Error();
+ },
----------------
dblaikie wrote:
> 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?
Basically enabling "rethrow"-like functionality. It applies in all the same cases that re-throw would be used.
I think I could add extra ErrorHandlerTraits specializations to detect void handlers and automatically return "Error::success()" into the handleErrors machinery.
================
Comment at: unittests/Support/ErrorTest.cpp:268-269
@@ +267,4 @@
+ // ordering.
+ if (CustomErrorInfo2 != 0)
+ abort();
+ CustomErrorInfo1 = CE.getInfo();
----------------
dblaikie wrote:
> Presumably these two lines should be an EXPECT_ or ASSERT_ style macro, rather than an anonymous abort()?
I wasn't sure about how gtest would handle nesting an EXPECT call inside another EXPECT. I'll give it a shot and if nothing explodes I'm happy to switch it over.
================
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));
+}
+}
+
----------------
dblaikie wrote:
> what's going on witmh the indentation and brace matching here?
clang-format. :)
Time to file a bug I guess.
================
Comment at: unittests/Support/ErrorTest.cpp:320
@@ +319,3 @@
+
+// Test that we can return values from handleErrors.
+{
----------------
dblaikie wrote:
> 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)
I'm not sure I follow that.
Error handlers can only return Error (or, if I implement the extension described earlier, void).
================
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:")
----------------
dblaikie wrote:
> You can just roll the contents of the lambda into the EXPECT_DEATH:
>
> EXPECT_DEATH({ Expected<int> A = make_error<CustomError>(42); }, ... );
Will do.
http://reviews.llvm.org/D17778
More information about the llvm-commits
mailing list