[PATCH] D92641: [llvm-readelf/obj] - Handle out-of-order PT_LOADs better.

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/unittests/Object/ELFObjectFileTest.cpp:377
+
+    EXPECT_TRUE((bool)DataOrErr);
+    EXPECT_TRUE(WarnString ==
----------------
jhenderson wrote:
> grimar wrote:
> > grimar wrote:
> > > jhenderson wrote:
> > > > I think if `DataOrErr` is false, we need to somehow handle the error, right? Actually maybe this should just be `ASSERT_TRUE`?
> > > I can't use `ASSERT_TRUE` here (or `ASSERT_THAT_EXPECTED`). Its intention is to return instantly,
> > > and so it calls `return` internally I think. And so I can't use it in lambda:
> > > ```
> > > Error	C2440	'return': cannot convert from 'void' to 'const uint8_t *'	ObjectTests	D:\Work3\LLVM\llvm-project\llvm\unittests\Object\ELFObjectFileTest.cpp	377	
> > > ```
> > > 
> > > > I think if DataOrErr is false, we need to somehow handle the error, right?
> > > 
> > > I don't think it is very important here? We expect `toMappedAddr` call to succeed here,
> > > this is what test is wrapped around. When something in a unit test goes wrong,
> > > we can probably just report and fail, no matter how.
> > > 
> > I can consume an error and `return nullptr` right after `EXPECT_TRUE((bool)DataOrErr)` probably.
> > It should be a bit cleaner. Should I?
> If you fail to consume the error in one form or other, this test will crash if the `DataOrErr` check fails, either from the unchecked error or the final dereference of `DataOrErr` at the end of the function, rather than producing a useful "test failed" type message. I don't think that's desirable. You could always try reporting the error via the `<<` syntax if the check fails before returning. I can't remember the exact syntax, but I believe it's something like `EXPECT_TRUE((bool)DataOrErr) << toString(DataOrErr.takeError());` That being said, shouldn't this be `EXPECT_THAT_EXPECTED(DataOrErr, Succeeded());`? I'm guessing that consumes the error safely (you'll still need to be careful about the dereference).
I've used `EXPECT_THAT_EXPECTED`. Yes, it calls `handleAllErrors` internally.




CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92641/new/

https://reviews.llvm.org/D92641