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

[Rahman Lavaee via Phabricator via llvm-commits]

rahmanl added a comment.

Thank you. Looks clean.



================
Comment at: llvm/include/llvm/Object/ELF.h:397
+  Expected<llvm::MapVector<const Elf_Shdr *, const Elf_Shdr *>>
+  getSectionAndRelocations(
+      std::function<Expected<bool>(const Elf_Shdr &)> IsMatch) const;
----------------
I think a function comment here will be very helpful. Something like:
```Returns a map from every section matching \p IsMatch to its relocation section, or \p nullptr if it has no relocation section. Returns error if any of the \p IsMatch calls fail or if it fails to retrieve the content section of any relocation section.```


================
Comment at: llvm/lib/Object/ELF.cpp:722
+    }
+    if (*DoesSectionMatch)
+      if (SecToRelocMap.insert(std::make_pair(&Sec, (const Elf_Shdr *)nullptr))
----------------
Use braces on the outer `if` to avoid a potential dangling `else` situation.


================
Comment at: llvm/lib/Object/ELF.cpp:747
+  }
+  if (Errors) {
+    return Errors;
----------------
remove these braces.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:6212
+    handleAllErrors(StackSizeRelocMapOrErr.takeError(), [&](StringError &SE) {
+      reportUniqueWarning(SE.getMessage());
+    });
----------------
Can we turn these into failures without breaking any tests? I think early return is not consistent with reporting warnings anymore.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144783