[PATCH] D68710: Remove time-trace message as it is inconsistent style

[Russell Gallop via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
russell.gallop marked an inline comment as done.
Closed by commit rG9d9ac46a08d7: Remove rest of time-trace message as it is 
inconsistent style (authored by russell.gallop).

Changed prior to commit:
  https://reviews.llvm.org/D68710?vs=224278=224290#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68710

Files:
  clang/include/clang/Driver/Options.td
  clang/tools/driver/cc1_main.cpp


Index: clang/tools/driver/cc1_main.cpp
===
--- clang/tools/driver/cc1_main.cpp
+++ clang/tools/driver/cc1_main.cpp
@@ -269,8 +269,6 @@
   // FIXME(ibiryukov): make profilerOutput flush in destructor instead.
   profilerOutput->flush();
   llvm::timeTraceProfilerCleanup();
-
-  llvm::errs() << "Time trace json-file dumped to " << Path.str() << "\n";
 }
   }
 
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -1791,7 +1791,12 @@
 def fthreadsafe_statics : Flag<["-"], "fthreadsafe-statics">, Group;
 def ftime_report : Flag<["-"], "ftime-report">, Group, 
Flags<[CC1Option]>;
 def ftime_trace : Flag<["-"], "ftime-trace">, Group,
-  HelpText<"Turn on time profiler">, Flags<[CC1Option, CoreOption]>;
+  HelpText<"Turn on time profiler. Generates JSON file based on output 
filename.">,
+  DocBrief<[{
+Turn on time profiler. Generates JSON file based on output filename. Results
+can be analyzed with chrome://tracing or `Speedscope App
+`_ for flamegraph visualization.}]>,
+  Flags<[CC1Option, CoreOption]>;
 def ftime_trace_granularity_EQ : Joined<["-"], "ftime-trace-granularity=">, 
Group,
   HelpText<"Minimum time granularity (in microseconds) traced by time 
profiler">,
   Flags<[CC1Option, CoreOption]>;


Index: clang/tools/driver/cc1_main.cpp
===
--- clang/tools/driver/cc1_main.cpp
+++ clang/tools/driver/cc1_main.cpp
@@ -269,8 +269,6 @@
   // FIXME(ibiryukov): make profilerOutput flush in destructor instead.
   profilerOutput->flush();
   llvm::timeTraceProfilerCleanup();
-
-  llvm::errs() << "Time trace json-file dumped to " << Path.str() << "\n";
 }
   }
 
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -1791,7 +1791,12 @@
 def fthreadsafe_statics : Flag<["-"], "fthreadsafe-statics">, Group;
 def ftime_report : Flag<["-"], "ftime-report">, Group, Flags<[CC1Option]>;
 def ftime_trace : Flag<["-"], "ftime-trace">, Group,
-  HelpText<"Turn on time profiler">, Flags<[CC1Option, CoreOption]>;
+  HelpText<"Turn on time profiler. Generates JSON file based on output filename.">,
+  DocBrief<[{
+Turn on time profiler. Generates JSON file based on output filename. Results
+can be analyzed with chrome://tracing or `Speedscope App
+`_ for flamegraph visualization.}]>,
+  Flags<[CC1Option, CoreOption]>;
 def ftime_trace_granularity_EQ : Joined<["-"], "ftime-trace-granularity=">, Group,
   HelpText<"Minimum time granularity (in microseconds) traced by time profiler">,
   Flags<[CC1Option, CoreOption]>;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68710: Remove time-trace message as it is inconsistent style

[Anton Afanasyev via Phabricator via cfe-commits]

anton-afanasyev accepted this revision.
anton-afanasyev added a comment.
This revision is now accepted and ready to land.

Yes, I agree. LGTM.


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

https://reviews.llvm.org/D68710



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68710: Remove time-trace message as it is inconsistent style

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop updated this revision to Diff 224278.
russell.gallop added a comment.

Update so --help helps user find trace file.
Documentation has the further details on how to view it.


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

https://reviews.llvm.org/D68710

Files:
  clang/include/clang/Driver/Options.td
  clang/tools/driver/cc1_main.cpp


Index: clang/tools/driver/cc1_main.cpp
===
--- clang/tools/driver/cc1_main.cpp
+++ clang/tools/driver/cc1_main.cpp
@@ -269,8 +269,6 @@
   // FIXME(ibiryukov): make profilerOutput flush in destructor instead.
   profilerOutput->flush();
   llvm::timeTraceProfilerCleanup();
-
-  llvm::errs() << "Time trace json-file dumped to " << Path.str() << "\n";
 }
   }
 
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -1793,7 +1793,12 @@
 def fthreadsafe_statics : Flag<["-"], "fthreadsafe-statics">, Group;
 def ftime_report : Flag<["-"], "ftime-report">, Group, 
Flags<[CC1Option]>;
 def ftime_trace : Flag<["-"], "ftime-trace">, Group,
-  HelpText<"Turn on time profiler">, Flags<[CC1Option, CoreOption]>;
+  HelpText<"Turn on time profiler.  Generates JSON file based on output 
filename.">,
+  DocBrief<[{
+Turn on time profiler. Generates JSON file based on output filename. Results
+can be analyzed with chrome://tracing or `Speedscope App
+`_ for flamegraph visualization.}]>,
+  Flags<[CC1Option, CoreOption]>;
 def ftime_trace_granularity_EQ : Joined<["-"], "ftime-trace-granularity=">, 
Group,
   HelpText<"Minimum time granularity (in microseconds) traced by time 
profiler">,
   Flags<[CC1Option, CoreOption]>;


Index: clang/tools/driver/cc1_main.cpp
===
--- clang/tools/driver/cc1_main.cpp
+++ clang/tools/driver/cc1_main.cpp
@@ -269,8 +269,6 @@
   // FIXME(ibiryukov): make profilerOutput flush in destructor instead.
   profilerOutput->flush();
   llvm::timeTraceProfilerCleanup();
-
-  llvm::errs() << "Time trace json-file dumped to " << Path.str() << "\n";
 }
   }
 
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -1793,7 +1793,12 @@
 def fthreadsafe_statics : Flag<["-"], "fthreadsafe-statics">, Group;
 def ftime_report : Flag<["-"], "ftime-report">, Group, Flags<[CC1Option]>;
 def ftime_trace : Flag<["-"], "ftime-trace">, Group,
-  HelpText<"Turn on time profiler">, Flags<[CC1Option, CoreOption]>;
+  HelpText<"Turn on time profiler.  Generates JSON file based on output filename.">,
+  DocBrief<[{
+Turn on time profiler. Generates JSON file based on output filename. Results
+can be analyzed with chrome://tracing or `Speedscope App
+`_ for flamegraph visualization.}]>,
+  Flags<[CC1Option, CoreOption]>;
 def ftime_trace_granularity_EQ : Joined<["-"], "ftime-trace-granularity=">, Group,
   HelpText<"Minimum time granularity (in microseconds) traced by time profiler">,
   Flags<[CC1Option, CoreOption]>;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68710: Remove time-trace message as it is inconsistent style

[Russell Gallop via Phabricator via cfe-commits]

russell.gallop marked 2 inline comments as done.
russell.gallop added inline comments.



Comment at: clang/include/clang/Driver/Options.td:1797
+  HelpText<"Turn on time profiler. Generates JSON file based on output 
filename. "
+   "Results can be analyzed with chrome://tracing or `Speedscope App "
+   "`_ for flamegraph visualization." >,

sylvestre.ledru wrote:
> I am not a big fan of adding products in the in-product help.
> If Chrome removes the feature or Speedscope goes done, we will still list 
> that in the old version of clang.
> Deprecated doc is more common and accepted.
> 
> I would remove this line and the next line and keep the doc.
> 
> I would remove this line and the next line and keep the doc.

I agree. Updated to do that via the tablegen file. Removed the changes to the 
rst file which is generated from tablegen.


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

https://reviews.llvm.org/D68710



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68710: Remove time-trace message as it is inconsistent style

[Sylvestre Ledru via Phabricator via cfe-commits]

sylvestre.ledru added a comment.

besides that, I think we should do it; thanks :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68710



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68710: Remove time-trace message as it is inconsistent style

[Sylvestre Ledru via Phabricator via cfe-commits]

sylvestre.ledru added inline comments.



Comment at: clang/include/clang/Driver/Options.td:1797
+  HelpText<"Turn on time profiler. Generates JSON file based on output 
filename. "
+   "Results can be analyzed with chrome://tracing or `Speedscope App "
+   "`_ for flamegraph visualization." >,

I am not a big fan of adding products in the in-product help.
If Chrome removes the feature or Speedscope goes done, we will still list that 
in the old version of clang.
Deprecated doc is more common and accepted.

I would remove this line and the next line and keep the doc.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68710



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits