[PATCH] D97094: [Driver] Print process statistics report on CC_PRINT_PROC_STAT env variable.

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

Thanks for the update @vvereschaka ! Just a few minor things and it should be good to go.
Also please use `git diff -U99999` next time to add context to the patch.



================
Comment at: clang/docs/UsersManual.rst:783
   It is possible to specify this option without any value. In this case statistics
   is printed on standard output in human readable format:
   
----------------
I think this should read //"In this case statistics **are** printed.."// Would you mind changing this as well?


================
Comment at: clang/docs/UsersManual.rst:799
+  setting the ``CC_PRINT_PROC_STAT`` and ``CC_PRINT_PROC_STAT_FILE`` environment
+  variables. Use ``CC_PRINT_PROC_STAT_FILE`` to provide a file name to store
+  the statistics.
----------------
Do you think it would be possible to rephrase a bit to avoid the repetition of `CC_PRINT_PROC_STAT_FILE`? Perhaps also explicitate that `CC_PRINT_PROC_STAT` is for "enabling the feature and logging to stdout"; while `CC_PRINT_PROC_STAT_FILE` only "redirects the log to a file".


================
Comment at: clang/lib/Driver/Driver.cpp:1108
+    CCPrintProcessStats = true;
+  Args.ClaimAllArgs(options::OPT_fproc_stat_report);
+
----------------
Remove this too, `hasArg` claims all arguments.


================
Comment at: clang/lib/Driver/Driver.cpp:4039
+
+      if (CCPrintStatReportFilename == nullptr) {
         using namespace llvm;
----------------
Suggestion: I find `if (!CCPrintStatReportFilename)` more intutive, more compact and easier to read, but that's my personal preference. There are arguments both ways probably, up to you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97094