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

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff marked 3 inline comments as done.
sepavloff 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
----------------
aganea wrote:
> Please add a header to the output .CSV, specifying the units of measure where relevant.
CSV was chosen because such file is a simple union of records. It is convenient if multiple processes write to it. A process locks the file, writes to it and unlock it.

If header is required, thing get more complicated. A process locks file, then moves to the end and get current position. If it is the beginning of the file, this process if the first writer, so it writes header. Then it writes line of data and unlock file.

On the other hand, CSV file is proposed for parsing by scripts. Does such complication really makes sense?


================
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
----------------
aganea wrote:
> MaskRay wrote:
> > aganea wrote:
> > > Why not just `-fproc-stat-report` in this case?
> > It can save an `Option`..
> I was thinking in terms of user experience, I find it a bit awkward. If you want stdin, you say `mytool < file`, if you want stdout you say `mytool` and pipe the output down the line. Is there a need to state the output when we're not writing to a file?
> 
> There is a precedent for this, other tools like xray have `llvm-xray account %s -o -`, but it's there for completness, you could just say `llvm-xray account %s`.
Agree, it looks awkward. I will add variant without value.


================
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
----------------
aganea wrote:
> MaskRay wrote:
> > aganea wrote:
> > > 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
> > > ```
> > Sorry, I tend to disagree with the argument for decimal separators and locale differences. They make behaviors divergent and make the output difficult to parse by a script. (Scripts may have to use `LANG=C clang -fproc-stat-report=-` to cancel the locale effect)
> In my sense, CSV or YAML are for machine parsing, TXT is for human consumption. A script should not parse a human-targetted output.
> Maybe an option is missing here to set the format? Again xray has `-f=text` or `-f=csv`.
I don't think an option for output format is needed here (but it could be useful if we supported JSON in addition to CSV). However the proposed format indeed is more readable. I am going to change printing accordingly, but without locale specifics. For users of this feature C locale is convenient.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78903