[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 ==
----------------
grimar wrote:
> 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.
> 
> 
I've found that `EXPECT_THAT_EXPECTED` doesn't help much here.

The problem is in the line below:
```
return !DataOrErr ? nullptr : *DataOrErr;
```

`EXPECT_THAT_EXPECTED` takes the error from `DataOrErr`,
but the unchecked bit is then set again by the operator bool:

```
  explicit operator bool() {
#if LLVM_ENABLE_ABI_BREAKING_CHECKS
    Unchecked = HasError;
#endif
    return !HasError;
  }
```

I think we should handle an error in a straighforward way here.



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

https://reviews.llvm.org/D92641