[PATCH] D66286: [llvm-readobj/llvm-readelf] - Improve/cleanup the error reporting API.

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:141-144
+      // TODO: Add a section index and the file name to this warning.
+      reportWarning(createError("invalid section size (" + Twine(Size) +
+                                ") or entity size (" + Twine(EntSize) + ")"),
+                    "<?>");
----------------
rupprecht wrote:
> I think `<?>` is confusing, and is worth fixing in this patch instead of deferring to a followup cleanup.
> 
> Could you perhaps make `DynRegionInfo` take a required filename parameter which can be initialized in `ELFDumper`'s constructor so it's always available?
Ok, done.
(FTR, "<?>" is used in COFFDumper.cpp, I took the idea from there.)




================
Comment at: tools/llvm-readobj/ELFDumper.cpp:4623
   uint64_t RelocSymValue = 0;
-  StringRef FileStr = Obj->getFileName();
+
   if (RelocSym != Obj->symbol_end()) {
----------------
MaskRay wrote:
> I think if a variable already exists, keeping it probably doesn't hurt. Typing `Obj->getFileName()` takes a bit more characters.
Ok.


================
Comment at: tools/llvm-readobj/ObjDumper.cpp:27
+static inline Error createError(const Twine &Err) {
+  return make_error<StringError>(Err, object::object_error::parse_failed);
+}
----------------
jhenderson wrote:
> Can you use `createStringError` here?
Done.


================
Comment at: tools/llvm-readobj/llvm-readobj.cpp:400
+                  [&](const ErrorInfoBase &EI) {
+                    fouts().flush();
+                    errs() << "\n";
----------------
rupprecht wrote:
> ... any idea why this was necessary before? This is a different stream. Might be nice to have a comment in the code. (Same for above in `error()`).
> ... any idea why this was necessary before? This is a different stream. 

Yes, I added it in D64472 because otherwise warnings reported could be printed at the wrong places.
(i.e. they were kind of randomly mixed to the standart output).

I added comments requested.


================
Comment at: tools/llvm-readobj/llvm-readobj.h:24-25
   // Various helper functions.
-  LLVM_ATTRIBUTE_NORETURN void reportError(Twine Msg);
   void reportError(Error Err, StringRef Input); 
-  void reportWarning(Twine Msg);
-  void reportWarning(StringRef Input, Error Err);
-  void warn(llvm::Error Err);
-  void error(std::error_code EC);
+  void reportError(std::error_code EC, StringRef Input);
+  void reportWarning(Error Err, StringRef Input);
----------------
rupprecht wrote:
> Can these both be `LLVM_ATTRIBUTE_NORETURN`? I think they should have been so before too.
Done.


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

https://reviews.llvm.org/D66286