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

[Ellis Hoag via Phabricator via llvm-commits]

ellis added inline comments.


================
Comment at: llvm/tools/llvm-profdata/llvm-profdata.cpp:60
 
+enum class ShowOutputFormat { Text, TextEncoding, Json, Yaml };
+
----------------
wenlei wrote:
> 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. 
I like your idea of `--show-format`. I've put up D135467 to rename the flag.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135127