<div dir="ltr">Hi Dave,<div><br></div><div>Thanks for the review. I've implemented most of your suggestions in r263764 and r263768.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Is there much need/benefit to being able to set the banner separately, rather than just always on construction?</blockquote><div><div> </div></div><div>Yes - I expect the idiomatic use of ExitOnError will be a global variable accessible everywhere in the tool, and initialized in main with the tool's name:</div><div><br></div><div><font face="monospace, monospace">ExitOnError ExitOnErr;</font></div><div><font face="monospace, monospace"><br></font></div><div><font face="monospace, monospace"><Lots of tool code></font></div><div><font face="monospace, monospace"><br></font></div><div><font face="monospace, monospace">int main(int argc, char *argv[0]) {</font></div><div><font face="monospace, monospace"> ExitOnErr.setBanner(std::string(argv[0]) + " error: "));</font></div><div><font face="monospace, monospace"> ...</font></div><div><font face="monospace, monospace">}<br></font></div><div><br></div><div>Cheers,</div><div>Lang.</div><div><br></div><div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Mar 17, 2016 at 2:45 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><div><div>On Thu, Mar 17, 2016 at 2:28 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:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Author: lhames<br>
Date: Thu Mar 17 16:28:49 2016<br>
New Revision: 263749<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=263749&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=263749&view=rev</a><br>
Log:<br>
[Support] Add ExitOnError utility to support tools that use the exit-on-error<br>
idiom.<br>
<br>
Most LLVM tool code exits immediately when an error is encountered and prints an<br>
error message to stderr. The ExitOnError class supports this by providing two<br>
call operators - one for Errors, and one for Expected<T>s. Calls to code that<br>
can return Errors (or Expected<T>s) can use these calls to bail out on error,<br>
and otherwise continue as if the operation had succeeded. E.g.<br>
<br>
Error foo();<br>
Expected<int> bar();<br>
<br>
int main(int argc, char *argv[]) {<br>
ExitOnError ExitOnErr;<br>
<br>
ExitOnErr.setBanner(std::string("Error in ") + argv[0] + ":");<br>
<br>
// Exit if foo returns an error. No need to manually check error return.<br>
ExitOnErr(foo());<br>
<br>
// Exit if bar returns an error, otherwise unwrap the contained int and<br>
// continue.<br>
int X = ExitOnErr(bar());<br>
<br>
// ...<br>
<br>
return 0;<br>
}<br>
<br>
<br>
Modified:<br>
llvm/trunk/include/llvm/Support/Error.h<br>
llvm/trunk/unittests/Support/ErrorTest.cpp<br>
<br>
Modified: llvm/trunk/include/llvm/Support/Error.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/Error.h?rev=263749&r1=263748&r2=263749&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/Error.h?rev=263749&r1=263748&r2=263749&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/include/llvm/Support/Error.h (original)<br>
+++ llvm/trunk/include/llvm/Support/Error.h Thu Mar 17 16:28:49 2016<br>
@@ -519,7 +519,8 @@ inline void handleAllErrors(Error E) {<br>
/// (allowing clean deallocation of resources, etc.), while reporting error<br>
/// information to the user.<br>
template <typename... HandlerTs><br>
-void logAllUnhandledErrors(Error E, raw_ostream &OS, std::string ErrorBanner) {<br>
+void logAllUnhandledErrors(Error E, raw_ostream &OS,<br>
+ const std::string &ErrorBanner) {<br>
if (!E)<br>
return;<br>
OS << ErrorBanner;<br>
@@ -742,6 +743,55 @@ inline std::error_code errorToErrorCode(<br>
return EC;<br>
}<br>
<br>
+/// Helper for check-and-exit error handling.<br>
+///<br>
+/// For tool use only. NOT FOR USE IN LIBRARY CODE.<br>
+///<br>
+class ExitOnError {<br>
+public:<br>
+<br>
+ /// Create an error on exit helper.<br>
+ ExitOnError(std::string Banner = "", int DefaultErrorExitCode = 1)<br>
+ : Banner(Banner),<br></blockquote><div><br></div></div></div><div>Missing std::move here ^</div><span><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
+ GetExitCode([=](const Error&) { return DefaultErrorExitCode; }) {}<br>
+<br>
+ /// Set the banner string for any errors caught by operator().<br></blockquote><div><br></div></span><div>Is there much need/benefit to being able to set the banner separately, rather than just always on construction?<br></div><span><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
+ void setBanner(std::string Banner) {<br>
+ this->Banner = Banner;<br></blockquote><div><br></div></span><div>Missing std::move here ^</div><span><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
+ } </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
+<br>
+ /// Set the exit-code mapper function.<br>
+ void setExitCodeMapper(std::function<int(const Error&)> GetExitCode) {<br>
+ this->GetExitCode = GetExitCode;<br></blockquote><div><br></div></span><div>Missing std::move here ^</div><span><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
+ }<br>
+<br>
+ /// Check Err. If it's in a failure state log the error(s) and exit.<br>
+ void operator()(Error Err) const {<br>
+ checkError(std::move(Err));<br>
+ }<br>
+<br>
+ /// Check E. If it's in a success state return the contained value. If it's<br>
+ /// in a failure state log the error(s) and exit.<br>
+ template <typename T><br>
+ T operator()(Expected<T> E) const {<br>
+ checkError(E.takeError());<br>
+ return std::move(*E);<br></blockquote><div><br></div></span><div>Maybe take E by rvalue ref (since you're not moving it, just bits of it, and T might be expensive to copy/non-movable)? Or do you need to be able to pass copies/lvalues?</div><div><div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
+ }<br>
+<br>
+private:<br>
+<br>
+ void checkError(Error Err) const {<br>
+ if (Err) {<br>
+ int ExitCode = GetExitCode(Err);<br>
+ logAllUnhandledErrors(std::move(Err), errs(), Banner);<br>
+ exit(ExitCode);<br>
+ }<br>
+ }<br>
+<br>
+ std::string Banner;<br>
+ std::function<int(const Error&)> GetExitCode;<br>
+};<br>
+<br>
} // namespace llvm<br>
<br>
#endif // LLVM_SUPPORT_ERROR_H<br>
<br>
Modified: llvm/trunk/unittests/Support/ErrorTest.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Support/ErrorTest.cpp?rev=263749&r1=263748&r2=263749&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Support/ErrorTest.cpp?rev=263749&r1=263748&r2=263749&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/unittests/Support/ErrorTest.cpp (original)<br>
+++ llvm/trunk/unittests/Support/ErrorTest.cpp Thu Mar 17 16:28:49 2016<br>
@@ -38,6 +38,7 @@ namespace {<br>
// - consume_error to consume a "safe" error without any output.<br>
// - handleAllUnhandledErrors to assert that all errors are handled.<br>
// - logAllUnhandledErrors to log errors to a stream.<br>
+// - ExitOnError tests.<br>
//<br>
// Expected tests:<br>
// - Expected<T> with T.<br>
@@ -50,6 +51,7 @@ namespace {<br>
// - std::error_code to Error (ECError) in failure mode.<br>
// - Error to std::error_code in success mode.<br>
// - Error (ECError) to std::error_code in failure mode.<br>
+//<br>
<br>
// Custom error class with a default base class and some random 'info' attached.<br>
class CustomError : public ErrorInfo<CustomError> {<br>
@@ -376,6 +378,32 @@ TEST(Error, CheckErrorUtilities) {<br>
EXPECT_EQ(ErrorInfo, 7)<br>
<< "Failed to handle Error returned from handleErrors.";<br>
}<br>
+<br>
+ // Test ExitOnError<br></blockquote><div><br></div></div></div><div>Probably good to pull it out into a separate test - more specific errors from gtest/lit, and it means less repetition when running those EXPECT_EXIT tests (each one has to be run in a separate process, and execute all the preamble in the same test case until tehy reach the unique EXPECT_EXIT they're intendting to run - so you might even consider splitting them out even further)</div><div><div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
+ {<br>
+ ExitOnError ExitOnErr;<br>
+ ExitOnErr.setBanner("Error in tool:");<br>
+ ExitOnErr.setExitCodeMapper(<br>
+ [](const Error &E) {<br>
+ if (E.isA<CustomSubError>())<br>
+ return 2;<br>
+ return 1;<br>
+ });<br>
+<br>
+ // Make sure we don't bail on success.<br>
+ ExitOnErr(Error::success());<br>
+ EXPECT_EQ(ExitOnErr(Expected<int>(7)), 7)<br>
+ << "exitOnError returned an invalid value for Expected";<br>
+<br>
+ // Exit tests.<br>
+ EXPECT_EXIT(ExitOnErr(make_error<CustomError>(7)),<br>
+ ::testing::ExitedWithCode(1), "Error in tool:")<br>
+ << "exitOnError returned an unexpected error result";<br>
+<br>
+ EXPECT_EXIT(ExitOnErr(Expected<int>(make_error<CustomSubError>(0, 0))),<br>
+ ::testing::ExitedWithCode(2), "Error in tool:")<br>
+ << "exitOnError returned an unexpected error result";<br>
+ }<br>
}<br>
<br>
// Test Expected behavior.<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">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>
</blockquote></div></div></div><br></div></div>
</blockquote></div><br></div></div></div>