[PATCH] D74308: [Debuginfo][NFC] Unify error reporting routines inside DebugInfoDWARF.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Overall, I like the principle of this change, but you should definitely hear what others have to say too.



================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h:355
+  };
+  std::function<void(Error E)> getRecoverableErrorHandler() {
+    return RecoverableErrorHandler;
----------------
Having a setter and getter for these seems a bit verbose, especially when you've got cases where you use the getter and then immediately call the return. Is there any particular reason to hide the handlers behind the interface?


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:362
+             Unit.getOrigUnit().getContext().getRecoverableErrorHandler(),
+             8 /* Indent */, DumpOpts);
   }
----------------
I know it's not related to your change, but since you are touching the line anyway, could you change `8 /* Indent */` to `/*Indent=*/8`, please, as clang-format plays well with that.


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:400
+    DIE.dump(outs(), OrigUnit.getContext().getRecoverableErrorHandler(),
+             8 /* Indent */, DumpOpts);
   }
----------------
Ditto


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDie.cpp:642
+LLVM_DUMP_METHOD void DWARFDie::dump() const {
+  dump(llvm::errs(), defaultErrorHandler, 0);
+}
----------------
Have you considered using DWARFContext::defaultErrorHandler here?


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp:240
+    Curr.dump(OS, DCtx.getRecoverableErrorHandler());
+    Die.dump(OS, DCtx.getRecoverableErrorHandler(), /*indent*/ 1);
     return 1;
----------------
Same comment as elsewhere re. "indent"


================
Comment at: llvm/tools/dsymutil/DwarfLinkerForBinary.cpp:162
   WithColor::note() << "    in DIE:\n";
-  DIE->dump(errs(), 6 /* Indent */, DumpOpts);
+  DIE->dump(errs(), ErrorHandler, 6 /* Indent */, DumpOpts);
 }
----------------
Same comments re. using DWARFContext::defaultErrorHandler and "Indent" comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74308