[PATCH] D144783: [ELF] Move getSectionAndRelocations to ELF.cpp from ELFDumper.cpp

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Code essentially looks good. I haven't had time to go through the tests yet, but if you plan on changing/expanding them anyway, I might as well wait until those are ready.



================
Comment at: llvm/lib/Object/ELF.cpp:713
+    llvm::MapVector<const Elf_Shdr *, const Elf_Shdr *> &SecToRelocMap) const {
+  Error IsSuccess = Error::success();
+  for (const Elf_Shdr &Sec : cantFail(this->sections())) {
----------------
I'm not sure this should be called `IsSuccess`, if I'm honest, since it's not a boolean. I'd name it something as simples as `Errors` or the like.


================
Comment at: llvm/lib/Object/ELF.cpp:730-733
+          make_error<StringError>(std::error_code(),
+                                  describe(*this, Sec) +
+                                      ": failed to get a relocated section: " +
+              toString(RelSecOrErr.takeError())));
----------------
I believe this can just be as the inline edit shows (reformatting will be required still).


================
Comment at: llvm/lib/Object/ELF.cpp:739
+    if(!DoesRelTargetMatch)
+      return DoesRelTargetMatch.takeError();
+    if (*DoesRelTargetMatch)
----------------
This will cause a failure in certain configurations, because `IsSuccess` is not being consumed in any way.

Is there a reason this and the other `return` statement in the loop don't simply use `joinErrors` like everything else?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144783