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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I got a few more minutes this morning, but that's it from me until the New Year. I don't mind this going in before then, once my comments have been addressed and somebody else reviews it.



================
Comment at: llvm/tools/llvm-elfabi/ErrorCollector.cpp:23
+
+void ErrorCollector::addError(Error Err, std::string Tag) {
+  if (Err) {
----------------
Should Err here be an r-value reference? I'm not sure copying it strictly makes sense. Tag be a StringRef (copy it at the point where necessary i.e. at the Tags.push_back() line).


================
Comment at: llvm/tools/llvm-elfabi/ErrorCollector.cpp:40
+  Errors.clear();
+  return createStringError(errc::interrupted, RawSS.str().c_str());
+}
----------------
errc::interrupted doesn't feel like the right answer here (what was interrupted?). This should probably be something else. I guess it may be worth checking the errc of of all errors and using that if it's the same, or picking one if not?


================
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)
----------------
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).


================
Comment at: llvm/tools/llvm-elfabi/ErrorCollector.cpp:53-56
+  if (Errors.empty())
+    return true;
+
+  return false;
----------------
`return Errors.empty();`


================
Comment at: llvm/tools/llvm-elfabi/ErrorCollector.cpp:70
+  errs() << "Aborted due to unhandled Error(s):\n";
+  log(errs());
+  abort();
----------------
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.


================
Comment at: llvm/tools/llvm-elfabi/ErrorCollector.h:62
+  /// false if errors have been added since the last makeError() call.
+  bool allErrsHandled() const;
+
----------------
Could we avoid the abbreviation here? Everywhere else it's Errors, not Errs (so this would be `allErrorsHandled`).


Repository:
  rL LLVM

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

https://reviews.llvm.org/D55352