[PATCH] D55352: [elfabi] Introduce tool for ELF TextAPI

[Armando Montanez via Phabricator via llvm-commits]

amontanez marked 3 inline comments as done.
amontanez added inline comments.


================
Comment at: llvm/tools/llvm-elfabi/ErrorCollector.cpp:46
+  for (size_t i = 0; i < Errors.size(); ++i) {
+    OS << i << " (" << Tags[i] << "): " << Errors[i];
+    if (i != Errors.size() - 1)
----------------
jhenderson wrote:
> Unless the indexes are going to be useful for something else, I don't think they give us anything. I also am not sure there's a need to print the "Encountered multiple errors bit and then print them all as a single error".
> 
> Instead, I'd print each of the errors separately (i.e. with the WithColor::error() method or equivalent), since each is a distinct message. One strong reason for doing this is that each is indeed a distinct error, so should be prefixed with "error:". This helps IDEs and the like display the different errors in a meaningful way (if you plugged this into Visual Studio for example, the Error List view would simply say "Encountered multiple errors" and you'd have to go to the build output to actually view them, whereas reporting them separately would result in separate errors being listed). Additionally, printing separate errors is easier to read when viewing the output in a terminal (because each is individually highlighted).
I've fixed this for `ErrorCollector::log()`, but it doesn't work as cleanly for the previous `ErrorCollector::makeError()` since color information would get stripped out. I've switched `ErrorCollector::makeError()` to use `joinErrors()` for now.


================
Comment at: llvm/tools/llvm-elfabi/ErrorCollector.cpp:70
+  errs() << "Aborted due to unhandled Error(s):\n";
+  log(errs());
+  abort();
----------------
jhenderson wrote:
> Does logging mark Error as checked? If not, we don't need the abort() (and can probably also get rid of the "Aborted due to...." message too) by simply skipping the consumeError() loop in the destructor, and letting the unchecked Error assertion fire as normal.
I tried omitting this and only one error is printed since the program terminates early when the first unhanded error is encountered.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D55352