[PATCH] D67182: [lib/ObjectYAML] - Improve and cleanup error reporting in ELFState<ELFT> class.

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: lib/ObjectYAML/ELFEmitter.cpp:416
 
-  return true;
+  return !HasError;
 }
----------------
jhenderson wrote:
> Maybe you could have this function return void, and then have the caller check `HasErrors`?
Yes, this is probably better.


================
Comment at: lib/ObjectYAML/ELFEmitter.cpp:453
-                           << "' by YAML symbol " << Sym.Name << ".\n";
-        exit(1);
-      }
----------------
jhenderson wrote:
> Adding a test case to show that we can report multiple errors for this issue would be good.
Done.


================
Comment at: lib/ObjectYAML/ELFEmitter.cpp:589
+template <class ELFT> void ELFState<ELFT>::reportError(const Twine &Msg) {
+  WithColor::error() << Msg << "\n";
+  HasError = true;
----------------
jhenderson wrote:
> This might be a bit of a bigger task than you had hoped for, and I'm okay if you want to push this suggestion to a later change, but it would be great if this could call some form of error handler passed in by the client of the library instead of directly calling `WithColor::error()`. That way the client can control how the errors are reported. See my lightning talk on error handling in libraries from the US conference a couple of years ago.
> 
> Regardless, setting `HasError` here seems fine, and obviously a default option of `WithColor::error()` is sensible.
I'll be happy to investigate it deeper and suggest a follow-up patch to add a custom error handler to this lib.


================
Comment at: lib/ObjectYAML/ELFEmitter.cpp:718-719
-    unsigned SymIdx = 0;
-    // If a relocation references a symbol, try to look one up in the symbol
-    // table. If it is not there, treat the value as a symbol index.
-    if (Rel.Symbol && !SymMap.lookup(*Rel.Symbol, SymIdx) &&
----------------
jhenderson wrote:
> Maybe it's worth having something like this comment in `toSymbolIndex`?
I added a comment.


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

https://reviews.llvm.org/D67182