[PATCH] D78903: [Driver] Add option -fproc-stat-report

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added inline comments.


================
Comment at: clang/docs/UsersManual.rst:770
+   $ cat abc
+   clang-11,"/tmp/foo-123456.o",92000,84000,87536
+   ld,"a.out",900,8000,53568
----------------
Please add a header to the output .CSV, specifying the units of measure where relevant.


================
Comment at: clang/docs/UsersManual.rst:786
+
+  $ clang -fproc-stat-report=- foo.c  
+  clang-11: output=/tmp/foo-123456.o, total=84000, user=76000, mem=87496
----------------
Why not just `-fproc-stat-report` in this case?


================
Comment at: clang/docs/UsersManual.rst:787
+  $ clang -fproc-stat-report=- foo.c  
+  clang-11: output=/tmp/foo-123456.o, total=84000, user=76000, mem=87496
+  ld: output=a.out, total=8000, user=8000, mem=53548
----------------
I think it is better if the units are specified along (and locale-formatted, if possible):
```
clang-11: output=/tmp/foo-123456.o  total=84,000 ms  user=76,000 ms  mem=87,496 kb
```


================
Comment at: clang/include/clang/Driver/Job.h:78
+  /// Information on executable run provided by OS.
+  mutable llvm::sys::ProcessStatistics ProcStat;
+
----------------
`mutable Optional<llvm::sys::ProcessStatistics> ProcStat;` and then you won't need `.isSet()`.


================
Comment at: clang/lib/Driver/Driver.cpp:3770
 
+  if (!StatReportFile.empty())
+    C.setPostCallback([=](const Command &Cmd, int Res) {
----------------
There is a large piece of code in the `if` statement, I would add curly brackets:
`if (!StatReportFile.empty()) { ... }`


================
Comment at: clang/lib/Driver/Driver.cpp:3772
+    C.setPostCallback([=](const Command &Cmd, int Res) {
+      const llvm::sys::ProcessStatistics &ProcStat = Cmd.getProcessStatistics();
+      if (ProcStat.isSet()) {
----------------
Use `Optional<>` like state above, then the condition changes to:
`if (ProcStat)`

and then usage below becomes:
`<< ", total=" << ProcStat->TotalTime.count()`


================
Comment at: clang/lib/Driver/Driver.cpp:3774
+      if (ProcStat.isSet()) {
+        if (StatReportFile.equals("-")) {
+          // Human readable output.
----------------
`if (StatReportFile.equals("-") || StatReportFile.empty()) {`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78903