[PATCH] D135127: [llvm-profdata] Add --output-format option

Fri Oct 7 10:03:33 PDT 2022

wenlei added inline comments.

================
Comment at: llvm/tools/llvm-profdata/llvm-profdata.cpp:60

+enum class ShowOutputFormat { Text, TextEncoding, Json, Yaml };
+
----------------
ellis wrote:
> MaskRay wrote:
> > Should this be `OutputFormat` instead? Then you can use a different name for the variable...
> I thought the name `OutputFormat` would be confusing because one might think it had the meaning of `ProfileFormat` above. `ProfileFormat` specifies which profile format we should output, while `ShowOutputFormat` only specifies the output for the show command. We don't currently have YAML or JSON profile formats.
`OutputFormat` is indeed confusing. Before you make show output format selectable, output format has one to one mapping with profile format, so output format and profile format were interchangeable.. And we have this line:

`cl::opt<ProfileFormat> OutputFormat`

We tried to hold an invariant -- all file outputs are valid profile, non-profile output are all for stdout (users can redirect to file). Can we try to hold that invariant still?  Can we disambiguate them by variable names as well as switch name? i.e. `--show-format` for `show` command, and `--output-format` for `merge` command. If you separate them in difference space, `--show-format=text` and `--output-format=text` could also co-exist if needed. 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135127