[llvm] r289017 - Unbreak buildbots where the debug info test was crashing due to unchecked error.

[David Blaikie via llvm-commits]

Oh, sorry - I see, you meant the macro that enables checking in
llvm::Error, right.

The thing is it doesn't respect your checking an Expected<T>, just checking
the llvm::Error.

So something like this. Actually, I think you don't even need the boolean
return:

  handleAllErrors(Expecte.takeError(), [](const llvm::ErrorInfoBase &EI) {
    FAIL() << EI.message();
  });

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)

https://github.com/google/googletest/blob/master/googletest/docs/Primer.md
https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuide.md

(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)


On Mon, Dec 12, 2016 at 2:38 PM Greg Clayton <gclayton at apple.com> wrote:

>
> > On Dec 12, 2016, at 2:30 PM, David Blaikie <dblaikie at gmail.com> wrote:
> >
> >
> >
> > On Mon, Dec 12, 2016 at 2:05 PM Greg Clayton <gclayton at apple.com> wrote:
> >
> > > On Dec 12, 2016, at 1:59 PM, David Blaikie <dblaikie at gmail.com> wrote:
> > >
> > >
> > >
> > > On Wed, Dec 7, 2016 at 6:21 PM Greg Clayton via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
> > > Author: gclayton
> > > Date: Wed Dec  7 20:11:03 2016
> > > New Revision: 289017
> > >
> > > URL: http://llvm.org/viewvc/llvm-project?rev=289017&view=rev
> > > Log:
> > > Unbreak buildbots where the debug info test was crashing due to
> unchecked error.
> > >
> > >
> > > Modified:
> > >     llvm/trunk/unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp
> > >
> > > Modified: llvm/trunk/unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp
> > > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp?rev=289017&r1=289016&r2=289017&view=diff
> > >
> ==============================================================================
> > > --- llvm/trunk/unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp
> (original)
> > > +++ llvm/trunk/unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp Wed
> Dec  7 20:11:03 2016
> > > @@ -51,14 +51,15 @@ Triple getHostTripleForAddrSize(uint8_t
> > >  /// \returns true if there were errors, false otherwise.
> > >  template <typename T>
> > >  static bool HandleExpectedError(T &Expected) {
> > > -  if (!Expected)
> > > -    return false;
> > >    std::string ErrorMsg;
> > >    handleAllErrors(Expected.takeError(), [&](const llvm::ErrorInfoBase
> &EI) {
> > >      ErrorMsg = EI.message();
> > >    });
> > > -  ::testing::AssertionFailure() << "error: " << ErrorMsg;
> > > -  return true;
> > > +  if (!ErrorMsg.empty()) {
> > > +    ::testing::AssertionFailure() << "error: " << ErrorMsg;
> > > +    return true;
> > > +  }
> > > +  return false;
> > >
> > > I think this could be written a bit simpler and without relying on the
> error string being non-empty, etc.
> > >
> > > Oh, huh - Lang, any ideas?
> > >
> > > 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.
> > >
> > > Lang, would it be more idiomatic here to test the error for
> non-failure ,return false, otherwise then handleAllErrors and return true.
> > >
> >
> > As you can see I was testing the error and returning false, but that was
> crashing some buildbots with some extra API macro defined.
> >
> > 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.
>
> 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.
>
> >
> > 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.
> >
> > Greg
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161212/beeb7e88/attachment.html>