[PATCH] D154754: [llvm-objdump] Change errors to warnings for symbol section name dumping

Mon Jul 10 10:49:23 PDT 2023

MaskRay added inline comments.

================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.h:120

+class Dumper {
+  const object::ObjectFile &O;
----------------
mtrofin wrote:
> MaskRay wrote:
> > mtrofin wrote:
> > > mtrofin wrote:
> > > > nit: `final`
> > > How about renaming this to `WarningsCollector` and scoping it down to just managing the warnings, and then the `print` functions that can tolerate failures take it as a parameter (instead of being its members)?
> > There is no virtual function so I think adding `final` has no performance advantage. I think we will soon derive `llvm::objdump::Dumper` like we do for `llvm::ObjDumper` (`llvm/tools/llvm-readobj/ObjDumper.h`). So adding `final` here will lead to one extra line diff in the future...
> > 
> > The name `Dumper` is to match llvm-readobj. In the future, we shall probably move more `print*` functions into member functions and extend `Dumper` with more states, so I think `Dumper` is proper.
> I see - could you put a TODO comment on top of `Dumper`, then, so the design intent is clear during the period when not all of the `print` functions have been moved - I assume doing that (the move) in this patch would be overkill?
Thank you! I am going to add the comment to `dumpObject` beside those `print*` free function calls to remind myself and future contributors.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154754