[PATCH] D86539: [Debuginfo][llvm-dwarfutil] llvm-dwarfutil dsymutil-like tool for ELF.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/tools/llvm-dwarfutil/llvm-dwarfutil.cpp:452-453
+
+// TODO: This implementation is copied from llvm-objcopy.
+//       It is nessessary to refactor it out and remove this duplication.
+static Error restoreStatOnFile(StringRef Filename,
----------------
avl wrote:
> jhenderson wrote:
> > avl wrote:
> > > jhenderson wrote:
> > > > Could we refactor it out first please? Alternatively, does the behaviour need to be in the initial landing of this patch, or can it be added later?
> > > I think it is better to land it in this form. It is useful functionality and it is better to have it from start. As to the refactoring restoreStatOnFile - I am going to create a patch for soon.
> > > As to the refactoring restoreStatOnFile - I am going to create a patch for soon.
> > 
> > Not sure if we're talking past each other hum. I'm simply requesting the `restoreStatOnFile` patch be created and then this patch rebased on top of it, rather than duplicating code that will be resolved by the other patch.
> I understand. This way current patch will depend on the patch for restoreStatOnFile. It looks non practical for me because it would delay review. This restoreStatOnFile is not an important interface thing which will make a lot of problems later. That is why I think it is better to not create patch dependencies and that it might be OK to do both things independently.
> 
> But if you prefer to refactor restoreStatOnFile first - will rebase this patch on requested refactoring.
I'd always prefer avoiding incurring technical debt like this when the way forward is not particularly complex. Besides, it's not really delaying this patch, as nobody has given the thumbs up to the core behaviour anyway (and it's not something that I'm prepared to do due to my lack of familiarity and time).


================
Comment at: llvm/tools/llvm-dwarfutil/llvm-dwarfutil.cpp:528
+  if (Error Err = validateAndSetOptions(Args, Opts)) {
+    error(std::move(Err), dwarfutil::ToolName);
+    return EXIT_FAILURE;
----------------
avl wrote:
> jhenderson wrote:
> > avl wrote:
> > > jhenderson wrote:
> > > > Many tools have the `error()` function call `std::exit(1)` to end the program without needing an explicit `return EXIT_FAILURE`. I'm okay either way with it, but wanted to raise common practice with you here.
> > > The reason to not calling std::exit(1) is that error() function  is used as a error reporting function for DWARFLinker. DWARFLinker in it`s current state does not stop if any error encountered. As an alternative I can create separate error reporting function for DWARFLinker.
> > I've got no preference, so do whatever you prefer. The one thing I would say: if you've printed `error: xxx` to stderr, people will expect the the tool to return with a non-zero exit code.
> that is already done:
> 
> 
> ```
> inline void error(Error Err, StringRef Prefix = "") {
>   handleAllErrors(std::move(Err), [&](ErrorInfoBase &Info) {
>     WithColor::error(errs(), Prefix) << Info.message() << '\n';
>   });
>   std::exit(EXIT_FAILURE);
> }
> ```
Yes, I saw, after I made this comment, but left the comment there as it's still relevant (just information now rather than something you need to change).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86539