<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Mon, Dec 12, 2016 at 3:37 PM Greg Clayton <<a href="mailto:gclayton@apple.com">gclayton@apple.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">The other thing is I don't want this to actually fail if the target isn't available (the most common error). </blockquote><div><br></div><div>Ah, I assumed the existing code:<br><br> ::testing::AssertionFailure() << "error: " << ErrorMsg;<br><br>Caused a failure in this case. Does it not?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">If you build the "ARM" targets only on x86 system, it will try to get the host architecture's 32 and 64 bit variants and either can fail, but we don't want this breaking the buildbots by failing a test. We only need a target to test 4 and 8 byte addresses and also because we need the whole target stack just to use the AsmPrinter so we can generate the DWARF. I am open to better ideas on how to fix this test to do something better. But we do want the majority of our buildbots to test the 4 and 8 byte addresses as much as possible. Right now if the target fails to be found, we just return early from the DWARF tests and don't create an error. The only other idea I have is to iterate through all targets and run all targets through the DWARF generator tests, but I didn't as that is probably not very inefficient use of testing time.<br class="gmail_msg">
<br class="gmail_msg">
Greg<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
> On Dec 12, 2016, at 2:54 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" class="gmail_msg" target="_blank">dblaikie@gmail.com</a>> wrote:<br class="gmail_msg">
><br class="gmail_msg">
> Oh, sorry - I see, you meant the macro that enables checking in llvm::Error, right.<br class="gmail_msg">
><br class="gmail_msg">
> The thing is it doesn't respect your checking an Expected<T>, just checking the llvm::Error.<br class="gmail_msg">
><br class="gmail_msg">
> So something like this. Actually, I think you don't even need the boolean return:<br class="gmail_msg">
><br class="gmail_msg">
> handleAllErrors(Expecte.takeError(), [](const llvm::ErrorInfoBase &EI) {<br class="gmail_msg">
> FAIL() << EI.message();<br class="gmail_msg">
> });<br class="gmail_msg">
><br class="gmail_msg">
> This should produce a fatal failure, which terminates gtest execution (with an exception or otherwise), so the code after this shouldn't need to conditionally return or anything - the fact that this function returns at all is sufficient. I can double check this for you if you like - but pretty sure that's how the gtest documentation reads. (FAIL is like ASSERT_TRUE(false) - assertions are fatal checks, terminating execution of the test)<br class="gmail_msg">
><br class="gmail_msg">
> <a href="https://github.com/google/googletest/blob/master/googletest/docs/Primer.md" rel="noreferrer" class="gmail_msg" target="_blank">https://github.com/google/googletest/blob/master/googletest/docs/Primer.md</a><br class="gmail_msg">
> <a href="https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuide.md" rel="noreferrer" class="gmail_msg" target="_blank">https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuide.md</a><br class="gmail_msg">
><br class="gmail_msg">
> (at this point, maybe the 3 lines are short enough you could put them inline ratehr than having a separate function - but maybe not? Up to you)<br class="gmail_msg">
><br class="gmail_msg">
><br class="gmail_msg">
> On Mon, Dec 12, 2016 at 2:38 PM Greg Clayton <<a href="mailto:gclayton@apple.com" class="gmail_msg" target="_blank">gclayton@apple.com</a>> wrote:<br class="gmail_msg">
><br class="gmail_msg">
> > On Dec 12, 2016, at 2:30 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" class="gmail_msg" target="_blank">dblaikie@gmail.com</a>> wrote:<br class="gmail_msg">
> ><br class="gmail_msg">
> ><br class="gmail_msg">
> ><br class="gmail_msg">
> > On Mon, Dec 12, 2016 at 2:05 PM Greg Clayton <<a href="mailto:gclayton@apple.com" class="gmail_msg" target="_blank">gclayton@apple.com</a>> wrote:<br class="gmail_msg">
> ><br class="gmail_msg">
> > > On Dec 12, 2016, at 1:59 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" class="gmail_msg" target="_blank">dblaikie@gmail.com</a>> wrote:<br class="gmail_msg">
> > ><br class="gmail_msg">
> > ><br class="gmail_msg">
> > ><br class="gmail_msg">
> > > On Wed, Dec 7, 2016 at 6:21 PM Greg Clayton via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" class="gmail_msg" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br class="gmail_msg">
> > > Author: gclayton<br class="gmail_msg">
> > > Date: Wed Dec 7 20:11:03 2016<br class="gmail_msg">
> > > New Revision: 289017<br class="gmail_msg">
> > ><br class="gmail_msg">
> > > URL: <a href="http://llvm.org/viewvc/llvm-project?rev=289017&view=rev" rel="noreferrer" class="gmail_msg" target="_blank">http://llvm.org/viewvc/llvm-project?rev=289017&view=rev</a><br class="gmail_msg">
> > > Log:<br class="gmail_msg">
> > > Unbreak buildbots where the debug info test was crashing due to unchecked error.<br class="gmail_msg">
> > ><br class="gmail_msg">
> > ><br class="gmail_msg">
> > > Modified:<br class="gmail_msg">
> > > llvm/trunk/unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp<br class="gmail_msg">
> > ><br class="gmail_msg">
> > > Modified: llvm/trunk/unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp<br class="gmail_msg">
> > > URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp?rev=289017&r1=289016&r2=289017&view=diff" rel="noreferrer" class="gmail_msg" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp?rev=289017&r1=289016&r2=289017&view=diff</a><br class="gmail_msg">
> > > ==============================================================================<br class="gmail_msg">
> > > --- llvm/trunk/unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp (original)<br class="gmail_msg">
> > > +++ llvm/trunk/unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp Wed Dec 7 20:11:03 2016<br class="gmail_msg">
> > > @@ -51,14 +51,15 @@ Triple getHostTripleForAddrSize(uint8_t<br class="gmail_msg">
> > > /// \returns true if there were errors, false otherwise.<br class="gmail_msg">
> > > template <typename T><br class="gmail_msg">
> > > static bool HandleExpectedError(T &Expected) {<br class="gmail_msg">
> > > - if (!Expected)<br class="gmail_msg">
> > > - return false;<br class="gmail_msg">
> > > std::string ErrorMsg;<br class="gmail_msg">
> > > handleAllErrors(Expected.takeError(), [&](const llvm::ErrorInfoBase &EI) {<br class="gmail_msg">
> > > ErrorMsg = EI.message();<br class="gmail_msg">
> > > });<br class="gmail_msg">
> > > - ::testing::AssertionFailure() << "error: " << ErrorMsg;<br class="gmail_msg">
> > > - return true;<br class="gmail_msg">
> > > + if (!ErrorMsg.empty()) {<br class="gmail_msg">
> > > + ::testing::AssertionFailure() << "error: " << ErrorMsg;<br class="gmail_msg">
> > > + return true;<br class="gmail_msg">
> > > + }<br class="gmail_msg">
> > > + return false;<br class="gmail_msg">
> > ><br class="gmail_msg">
> > > I think this could be written a bit simpler and without relying on the error string being non-empty, etc.<br class="gmail_msg">
> > ><br class="gmail_msg">
> > > Oh, huh - Lang, any ideas?<br class="gmail_msg">
> > ><br class="gmail_msg">
> > > It'd be nice if handleAllErrors could return a value (if all handlers returned the same/compatibible) - though I guess then it'd need a default value.<br class="gmail_msg">
> > ><br class="gmail_msg">
> > > Lang, would it be more idiomatic here to test the error for non-failure ,return false, otherwise then handleAllErrors and return true.<br class="gmail_msg">
> > ><br class="gmail_msg">
> ><br class="gmail_msg">
> > As you can see I was testing the error and returning false, but that was crashing some buildbots with some extra API macro defined.<br class="gmail_msg">
> ><br class="gmail_msg">
> > Got a link/quote of a buildbot failure - that sounds like a rather different problem from what this code would address, so I'm probably not picturing what sort of failure you're describing.<br class="gmail_msg">
><br class="gmail_msg">
> without the extra API define, the initial code works fine. With the extra API define, we get an error stating no one checked the error.<br class="gmail_msg">
><br class="gmail_msg">
> ><br class="gmail_msg">
> > I have been looking for Lang around here but he is on vacation. As soon as he gets back I will check with him and make this work, but for now I didn't want buildbots to be broken.<br class="gmail_msg">
> ><br class="gmail_msg">
> > Greg<br class="gmail_msg">
><br class="gmail_msg">
<br class="gmail_msg">
</blockquote></div></div>