[PATCH] D69671: [llvm-readobj] Change errors to warnings for symbol section name dumping

[James Henderson via Phabricator via llvm-commits]

jhenderson marked 2 inline comments as done.
jhenderson added inline comments.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:562
+template <class ELFT>
+void DumpStyle<ELFT>::reportUniqueWarning(Error Err) const {
+  handleAllErrors(std::move(Err), [&](const ErrorInfoBase &EI) {
----------------
grimar wrote:
> Our warnings are always unique. Should it be just `reportWarning` (it is much more common name for reporting)?
At the moment, nothing really forces our warnings to be unique. The llvm-readobj.h `reportWarning` function does not do any uniquifying, so if the same warning could be reported from two different places, it isn't unique. I think one such example where it isn't is `getSymbolForReloc`. There are others too in the stack size, addrsig and note printing code. Probably we could change all of these to call the new function.

A concern with calling the new function `reportWarning` is that it could easily clash with the `reportWarning` in llvm-readobj.h, and people could end up calling the wrong one. I'm not sure of a good way to resolve this, as `reportUniqueWarning` could be easily missed in the future. What do you think?


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:840
+    if (!SectionIndex)
+      return "<?>";
+    Expected<StringRef> NameOrErr = getSymbolSectionName(Symbol, *SectionIndex);
----------------
grimar wrote:
> I think you need to `consumeError()` here.
Yup, sorry! This should probably actually print a warning...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69671