<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Mar 14, 2016 at 5:29 PM, Lang Hames via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">lhames marked 2 inline comments as done.<br>
<div><div class="h5"><br>
================<br>
Comment at: include/llvm/Support/Error.h:349-379<br>
@@ +348,33 @@<br>
+<br>
+// Specialization for member functions of the form 'Error (const ErrT&)'.<br>
+template <typename C, typename ErrT><br>
+class ErrorHandlerTraits<Error (C::*)(ErrT &)><br>
+ : public ErrorHandlerTraits<Error (&)(ErrT &)> {};<br>
+<br>
+// Specialization for member functions of the form 'Error (const ErrT&) const'.<br>
+template <typename C, typename ErrT><br>
+class ErrorHandlerTraits<Error (C::*)(ErrT &) const><br>
+ : public ErrorHandlerTraits<Error (&)(ErrT &)> {};<br>
+<br>
+// Specialization for member functions of the form 'Error (const ErrT&)'.<br>
+template <typename C, typename ErrT><br>
+class ErrorHandlerTraits<Error (C::*)(const ErrT &)><br>
+ : public ErrorHandlerTraits<Error (&)(ErrT &)> {};<br>
+<br>
+// Specialization for member functions of the form 'Error (const ErrT&) const'.<br>
+template <typename C, typename ErrT><br>
+class ErrorHandlerTraits<Error (C::*)(const ErrT &) const><br>
+ : public ErrorHandlerTraits<Error (&)(ErrT &)> {};<br>
+<br>
+/// Specialization for member functions of the form<br>
+/// 'Error (std::unique_ptr<ErrT>) const'.<br>
+template <typename C, typename ErrT><br>
+class ErrorHandlerTraits<Error (C::*)(std::unique_ptr<ErrT>)><br>
+ : public ErrorHandlerTraits<Error (&)(std::unique_ptr<ErrT>)> {};<br>
+<br>
+/// Specialization for member functions of the form<br>
+/// 'Error (std::unique_ptr<ErrT>) const'.<br>
+template <typename C, typename ErrT><br>
+class ErrorHandlerTraits<Error (C::*)(std::unique_ptr<ErrT>) const><br>
+ : public ErrorHandlerTraits<Error (&)(std::unique_ptr<ErrT>)> {};<br>
+<br>
----------------<br>
</div></div><span class="">dblaikie wrote:<br>
> Where's all this get used? Didn't immediately spot any test cases related to member function error handlers.<br>
</span>Lambdas. :)<br>
<br>
The six empty specializations are for (in order)<br>
<br>
[](ErrT&) mutable { ... }<br>
[](ErrT&) { ... }<br>
[](const ErrT&) mutable { ... }<br>
[](const ErrT&) { ... }<br>
[](std::unique_ptr<ErrT>) mutable { ... }<br>
[](std::unique_ptr<ErrT>) { ... }<br>
<br>
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).<br>
<br>
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.<br>
<span class=""><br>
================<br>
Comment at: include/llvm/Support/Error.h:447-461<br>
@@ +446,17 @@<br>
+<br>
+ // Helper class to log uncaught errors.<br>
+ class LogUncaughtErrorsTo {<br>
+ public:<br>
+ typedef ErrorInfoBase ErrorT;<br>
+ LogUncaughtErrorsTo(raw_ostream &OS) : OS(OS) {}<br>
+ bool appliesTo(ErrorInfoBase &EI) const { return true; }<br>
+ Error operator()(std::unique_ptr<ErrorInfoBase> EI) {<br>
+ EI->log(OS);<br>
+ OS << "\n";<br>
+ return Error();<br>
+ }<br>
+<br>
+ private:<br>
+ raw_ostream &OS;<br>
+ };<br>
+<br>
----------------<br>
</span><span class="">dblaikie wrote:<br>
> Can this not be implemented with a lambda like other handlers? (something about the "appliesTo" functionality?)<br>
</span>Indeed it can - this is a hangover from the earlier design. I'll remove it.<br>
<span class=""><br>
================<br>
Comment at: include/llvm/Support/Error.h:616-617<br>
@@ +615,4 @@<br>
+<br>
+ this->~Expected();<br>
+ new (this) Expected(std::move(Other));<br>
+ }<br>
----------------<br>
</span><span class="">dblaikie wrote:<br>
> That feels a tad heavyweight & perhaps nicer to do with just a couple of named functions, but I'm not wedded to the notion.<br>
</span>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.<br>
<span class=""><br>
================<br>
Comment at: unittests/Support/ErrorTest.cpp:148<br>
@@ +147,3 @@<br>
+<br>
+ consumeError(std::move(E));<br>
+ consumeError(std::move(F));<br>
----------------<br>
</span><span class="">dblaikie wrote:<br>
> 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.<br>
</span>Sounds reasonable. I'll add that.<br>
<span class=""><br>
================<br>
Comment at: unittests/Support/ErrorTest.cpp:181<br>
@@ +180,3 @@<br>
+<br>
+ // Check that named handlers of type 'Error (const Err&)' work.<br>
+ handleAllErrors(make_error<CustomError>(42), handleCustomError);<br>
----------------<br>
</span><span class="">dblaikie wrote:<br>
> 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?<br>
</span>Named handlers are function pointers, whereas lambdas are member functions. These tests are exercising different ErrorHandlerTraits specializations.<br></blockquote><div><br></div><div>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).<br><br>I'll stare at this code harder.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
================<br>
Comment at: unittests/Support/ErrorTest.cpp:257-272<br>
@@ +256,18 @@<br>
+<br>
+ handleAllErrors(std::move(E),<br>
+ [&](const CustomSubError &SE) {<br>
+ CustomErrorInfo2 = SE.getInfo();<br>
+ CustomErrorExtraInfo = SE.getExtraInfo();<br>
+ return Error();<br>
+ },<br>
+ [&](const CustomError &CE) {<br>
+ // Assert that the CustomError instance above is handled<br>
+ // before the<br>
+ // CustomSubError - joinErrors should preserve error<br>
+ // ordering.<br>
+ if (CustomErrorInfo2 != 0)<br>
+ abort();<br>
+ CustomErrorInfo1 = CE.getInfo();<br>
+ return Error();<br>
+ });<br>
+<br>
----------------<br>
</span><span class="">dblaikie wrote:<br>
> 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<br>
</span>This is actually clang-format output, and I think it gets it right here.<br></blockquote><div><br></div><div>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:<br><br>bool b = std::any_of(x.begin, x.end(), [&](const T& t) {<br> ...<br>});<br><br>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) </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Very short lambdas may get put on the same line, which looks odd - I could suggest that they specialize that.<br>
<span class=""><br>
================<br>
Comment at: unittests/Support/ErrorTest.cpp:261<br>
@@ +260,3 @@<br>
+ CustomErrorExtraInfo = SE.getExtraInfo();<br>
+ return Error();<br>
+ },<br>
----------------<br>
</span><span class="">dblaikie wrote:<br>
> why are the error handlers returning errors? (maybe I'll figure this out before I finish reviewing the patch...)<br>
><br>
> 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?<br>
</span>Basically enabling "rethrow"-like functionality. It applies in all the same cases that re-throw would be used.<br>
<br>
I think I could add extra ErrorHandlerTraits specializations to detect void handlers and automatically return "Error::success()" into the handleErrors machinery.<br></blockquote><div><br></div><div>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... ))</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
<br>
================<br>
Comment at: unittests/Support/ErrorTest.cpp:268-269<br>
@@ +267,4 @@<br>
+ // ordering.<br>
+ if (CustomErrorInfo2 != 0)<br>
+ abort();<br>
+ CustomErrorInfo1 = CE.getInfo();<br>
----------------<br>
</span><span class="">dblaikie wrote:<br>
> Presumably these two lines should be an EXPECT_ or ASSERT_ style macro, rather than an anonymous abort()?<br>
</span>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.<br>
<span class=""><br>
================<br>
Comment at: unittests/Support/ErrorTest.cpp:279-287<br>
@@ +278,11 @@<br>
+<br>
+ // Test consumeError for both success and error cases.<br>
+ {{Error E;<br>
+ consumeError(std::move(E));<br>
+}<br>
+{<br>
+ Error E = make_error<CustomError>(7);<br>
+ consumeError(std::move(E));<br>
+}<br>
+}<br>
+<br>
----------------<br>
</span><span class="">dblaikie wrote:<br>
> what's going on witmh the indentation and brace matching here?<br>
</span>clang-format. :)<br>
<br>
Time to file a bug I guess.<br>
<span class=""><br>
================<br>
Comment at: unittests/Support/ErrorTest.cpp:320<br>
@@ +319,3 @@<br>
+<br>
+// Test that we can return values from handleErrors.<br>
+{<br>
----------------<br>
</span><span class="">dblaikie wrote:<br>
> 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)<br>
</span>I'm not sure I follow that.<br>
<br>
Error handlers can only return Error (or, if I implement the extension described earlier, void).<br>
<span class=""><br>
================<br>
Comment at: unittests/Support/ErrorTest.cpp:382-385<br>
@@ +381,6 @@<br>
+ {<br>
+ auto DropUncheckedExpected = []() {<br>
+ Expected<int> A = make_error<CustomError>(42);<br>
+ };<br>
+ EXPECT_DEATH(DropUncheckedExpected(),<br>
+ "Program aborted due to an unhandled Error:")<br>
----------------<br>
</span><span class="">dblaikie wrote:<br>
> You can just roll the contents of the lambda into the EXPECT_DEATH:<br>
><br>
> EXPECT_DEATH({ Expected<int> A = make_error<CustomError>(42); }, ... );<br>
</span>Will do.<br>
<div class="HOEnZb"><div class="h5"><br>
<br>
<a href="http://reviews.llvm.org/D17778" rel="noreferrer" target="_blank">http://reviews.llvm.org/D17778</a><br>
<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div><br></div></div>