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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay 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()) {
----------------
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.


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

https://reviews.llvm.org/D64470