[PATCH] D79398: [WIP][dsymutil] Add reproducers to dsymutil

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere marked 5 inline comments as done.
JDevlieghere added inline comments.


================
Comment at: llvm/tools/dsymutil/Options.td:157
 
+def gen_reproducer: F<"gen-reproducer">,
+  HelpText<"Generate a reproducer consisting of the input object files.">,
----------------
aprantl wrote:
> is there a reason why this isn't called `--generate-reproducer`? People are not going to call this very often (I hope!) so we should optimize for legibility?
I did it for consistently with clang, which has the same flag. 


================
Comment at: llvm/tools/dsymutil/Options.td:161
+
+def use_reproducer: Separate<["--", "-"], "use-reproducer">,
+  MetaVarName<"<path>">,
----------------
aprantl wrote:
> What about `--record-reproducer` and `--replay-reproducer`?
It's funny you say this because that's what I had originally. It would certainly be more consistent with `lldb`. I changed it because I don't think "replay" is accurate for dsymutil. I don't plan on capturing anything beyond the input files, so really there's nothing being "replayed". The functionality is more closely related to what `clang` does, which is why I went with "generate" instead of "capture". 


================
Comment at: llvm/tools/dsymutil/Reproducer.cpp:41
+  FC->writeMapping(Mapping.str());
+  errs() << "Reproducer written to " << Root << '\n';
+}
----------------
avl wrote:
> It looks like it is not an error, but just logging. Thus it should probably be put under Verbose flag?
> 
> 
> ```
> if (Options.Verbose)
>       outs() << "Reproducer written"
> ```
It's more than logging, it's the only way to find out where the reproducer has been written. I'll change it to `outs()`, but I don't think it should be behind the `Verbose` flag. 





Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79398