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

[George Rimar via Phabricator via llvm-commits]

grimar 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) {
----------------
jhenderson wrote:
> 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?
> At the moment, nothing really forces our warnings to be unique. 

Yeah, I meant ones reported with the use of `WarningHandler`. I've not realized it is almost never used still.

> I'm not sure of a good way to resolve this, as reportUniqueWarning could be easily missed in the future. 

So currently we have a global `reportWarning(Error Err, StringRef Input)` and this new `reportUniqueWarning(Error Err)`.

I am thinking about the following steps:
1) We can switch all places that use `reportWarning(Error Err, StringRef Input)` to use `reportUniqueWarning(Error Err)` where possible/reasonable/easy (sometimes seems it is not that simple atm).
It looks like this might cover most of the places => will make normal for warnings to be unique.

2) We can rename `reportWarning(Error Err, StringRef Input)` to `reportNonUniqueWarning(Error Err, StringRef Input)` to emphasize there is something unusual/wrong with it. It is actually is, right? We probably want to report only unique warnings everywhere or almost everywhere.

3) And then we can rename `reportUniqueWarning` to just `reportWarning`, it will suggest it to be our normal warning reporting API.

How does it sound?

Anyways seems for now it is OK to keep the name of `reportUniqueWarning` as is, as the problem is a bit more complex as I supposed initially.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69671