[PATCH] D64014: [Object/ELF.h] - Improve error reporting.

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: include/llvm/Object/ELF.h:63
+    return "[" + std::to_string(Sec - &(*TableOrErr).front()) + "]";
+  llvm::consumeError(TableOrErr.takeError());
+  return "[?]";
----------------
jhenderson wrote:
> MaskRay wrote:
> > grimar wrote:
> > > jhenderson wrote:
> > > > Why not have this return an Expected, and let the call sites handle the error rather than throwing it away here? Of course, they could throw it away there.
> > > Two reasons:
> > > 1) To simplify the implementation. With the current version it is a very convenient helper that does not need additional wrapping.
> > >      It also allows to build the final string of view `[index X]` in one place.
> > > 2) I believe this error will *never* happen actually. Because our code always calls `sections()` earlier than this method is called,
> > > so it just should succeed here.
> > > But I still had to handle the error somehow and decided that because of all above the best way is just to return "[?]" as a section index in this
> > > really unlikely and untestable (I believe) case.
> > > 
> > The use of consumeError deserves a comment (the comment of the function says it may indicate a design problem)
> Whilst what you're saying is fine for now, I can imagine in the future that people will want to call this function in other contexts beyond error handling. As such, it needs future proofing. At least a comment at the signature level saying that any unknown indexes are ignored would be good or changing the function name to something like "getSecIndexForError" would be good.
I renamed it.


================
Comment at: test/Object/invalid.test:48
 
-# RELA: Section is not SHT_RELA
+# RELA: LLVM ERROR: Section is not SHT_RELA
 
----------------
jhenderson wrote:
> Not to be fixed now, but it would be great if this didn't use report_fatal_error, since it's not really an internal error in most cases.
Yes, I noticed this place. There are many things I am planning to improve in the follow-ups for error-reporting, and this is one of them.


================
Comment at: test/Object/invalid.test:468
 
-# RELOC-BROKEN-ENTSIZE: Invalid entity size
+# RELOC-BROKEN-ENTSIZE: error: Invalid entity size
 
----------------
jhenderson wrote:
> Report/check the invalid size?
No :) Because I am not changing this error message in this patch. (I only added a `error:` prefix to this test case).


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

https://reviews.llvm.org/D64014