[PATCH] D17778: TypedError for recoverable error handling
David Blaikie via llvm-commits
llvm-commits at lists.llvm.org
Mon Mar 14 17:49:13 PDT 2016
On Mon, Mar 14, 2016 at 5:29 PM, Lang Hames via llvm-commits <
llvm-commits at lists.llvm.org> wrote:
> 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.
>
This bit confuses me - I wouldn't expect a generic API to have any code
that would be specific to different kinds of functors (ie: anything that's
different between a function object and a function pointer), except for
optimization purposes (eg: small object optimizations, template
instantiation collapsing, etc).
I'll stare at this code harder.
>
> ================
> 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.
>
clang-format has a special case for "ending with a lambda when the start of
the lambda/function call is on a single line" good for things like:
bool b = std::any_of(x.begin, x.end(), [&](const T& t) {
...
});
And I'm wondering if that could/should be extended to multiple, like in
this case (though in this case the lambda parameter declarations/signature
are probably too long to fit on the tail of the first line, unfortunately -
not sure what to do about that)
>
> 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.
>
Oh, so they can only return errors? It sounded like they could return
anything? (is there a reason it should only be errors? (is there a reason
this needs to be in the machinery at all, rather than just using a capture:
Error e; handleErrors(.... lambdas store in 'e' if they fail to handle &
want to give something back ...) etc... ))
>
>
> ================
> 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
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160314/31df291c/attachment.html>
More information about the llvm-commits
mailing list