[PATCH] D64470: [Object/ELF] - Improve error reporting for notes.

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: include/llvm/Object/ELF.h:225
+    assert(Shdr.sh_type == ELF::SHT_NOTE && "Shdr is not of type SHT_NOTE");
+    assert(!Err && "Err should have Error::success() value");
     if (Shdr.sh_offset + Shdr.sh_size > getBufSize()) {
----------------
MaskRay wrote:
> grimar wrote:
> > MaskRay wrote:
> > > Alternatively,
> > > 
> > > ```
> > > assert(Shdr.sh_type == ELF::SHT_NOTE && "Shdr is not of type SHT_NOTE");
> > > if (Err)
> > >   return Elf_Note_Iterator(); // end() if Err is set
> > > if (Shdr.sh_offset + Shdr.sh_size > getBufSize()) {
> > >   ...
> > > }
> > > return Elf_Note_Iterator(base() + Shdr.sh_offset, Shdr.sh_size, Err);
> > > ```
> > But that means that `Err` can be set to something before the call.
> > I do not think it is the behavior we want actually.
> > Our code written currently in the way that Err is always set to success before the call.
> > So we assume there is no unhandled error. But imagine we have one because of any reason.
> > 
> > Then 
> > 
> > ```
> > if (Err)
> >   return Elf_Note_Iterator(); // end() if Err is set
> > ```
> > 
> > Will mark it as checked and that can hide the error.
> > In my version an assert will be triggered in that case, I think it is better. What do you think?
> > In my version an assert will be triggered in that case, I think it is better. What do you think?
> 
> I agree. Wait, I think there might be a better version:
> 
> ```
>   ErrorAsOutParameter ErrAsOutParam(&Err);
> ```
> 
> What I worry about `assert(!Err && "Err should have Error::success() value");` is that it sets the check bit in an assertion build.
> What I worry about assert(!Err && "Err should have Error::success() value"); is that it sets the check bit in an assertion build.

True. This could be be solved if Error::getPtr() wasn't private: `assert(Err.getPtr() && ..`
I think in some cases it might be useful to have an ability to assert.

> ErrorAsOutParameter

Cool, I did not know about this one, I used it, thanks!



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

https://reviews.llvm.org/D64470