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

[George Rimar via Phabricator via llvm-commits]

grimar marked an inline comment as done.
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:
> 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?


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

https://reviews.llvm.org/D64470