[PATCH] D71060: [LLD][ELF] Add time-trace to ELF LLD (2/2)

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: lld/ELF/Driver.cpp:533
+    SmallString<128> path(config->outputFile);
+    llvm::sys::path::replace_extension(path, "json");
+    if (!args.getLastArgValue(OPT_time_trace_eq).empty())
----------------
russell.gallop wrote:
> ruiu wrote:
> > I think it is better to make -time-trace to just print the result to stdout, as it is convenient.
> Hmm, I went for this as it was more consistent with the comparable compiler option.
> 
> I do find this behaviour useful if you are tracing a build such as llvm which has a lot of links. While this probably isn't typical, it does make it easy to send all of the link traces to separate files just by adding a single link option to all links.
> 
> What about an option like "-ftime-trace -" to send to stdout?
Yeah that makes sense, but I think that ".json" extension is the problem. We may want to add a different feature that outputs some data in the JSON format, and if we choose the same design, the option will conflict with the feature that you are adding. How about adding ".time-trace" to an output file then? Also, I don't think we should replace an extension -- usually Unix commands don't have extensions. I'd just append ".time-trace"


================
Comment at: lld/ELF/Options.td:367-368
 
+def time_trace: F<"time-trace">, HelpText<"Record time trace">;
+def time_trace_eq: J<"time-trace=">, HelpText<"Record time trace to specified file">;
+
----------------
I think that giving two options the same name isn't very conventional as a Unix command, as `--foo bar` and `--foo=bar` are usually considered the same option. Could you rename the latter `--time-trace-file`?


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

https://reviews.llvm.org/D71060