[PATCH] D97265: [clang] Allow clang-check to customize output file name

[Ella Ma via Phabricator via cfe-commits]

OikawaKirie added a comment.

In D97265#2581653 <https://reviews.llvm.org/D97265#2581653>, @sammccall wrote:

> 



> It looks like either we're in syntax-only mode (and -o will be ignored regardless) or we're in analyzer mode, and want -o.
> It seems tempting to just stop stripping it in this case, but then we'll end up with -o foo.o -o foo.html, which is no good.
> So the logic of this patch seems sound.

The strip output adjuster is invoked before appending the new `-o` option (line 215). Therefore, there will be only one `-o` option left if the new `-output` option is set when running `clang-check`.

> I wonder about the CLI though, I think the user will have to pass four flags with different styles to use it!
>
> `clang-check -analyze ... -extra-arg=-Xanalyzer -extra-arg=-analyzer-output=html -output=html_output ....`
>
> (Maybe -analyze should imply -Xanalyzer, and analyzer-output should be promoted to a real flag without the legacy plist default...)

You are right. It is better if we can add the `clang-check` version of `-Xanalyzer` options. However, the analyzer options can only determine the format of the output, rather than the name of the output. Therefore, we still need another option to set the output name.

My original idea is to add a generic way for the `clang-check` users to customize the output name. Although there is currently only one user, the static analyzer, there may be other users of this option in the future. Therefore, I thought that it would be better to add just a generic output option here.

According to your suggestions, it seems to be a good idea to use `-Xanalyzer` to set the output name, and the users can have a good experience when using clang-check to analyze their code. However, the disadvantage of this method is that the argument parsing procedure will become more complex. IMO, we can also add a `clang-check` option to set the analyzer output format together with the analyzer output name, and leave other analyzer options that are not commonly used to the `-Xanalyzer` option (refer to the second inline comment).



================
Comment at: clang/test/Tooling/clang-check-reset-o.cpp:10
+// CHECK: {{qwerty}}
+// CHECK: C++ requires
+invalid;
----------------
sammccall wrote:
> it looks like you're checking for the analysis finding in stdout of clang-check, rather than in the generated html file.
> So i'm not sure this test verifies it actually works!
Different kinds of output formats can be either a directory or a file named after the `-o` option. It is inconvenient to check the exact output file or directory.

This test case is created by imitating the `clang/test/Tooling/clang-check-strip-o.cpp` test case, which only checks whether the cc1 arguments are correctly set.




================
Comment at: clang/tools/clang-check/ClangCheck.cpp:80
+static cl::opt<std::string>
+    Output("output", cl::desc(Options.getOptionHelpText(options::OPT_o)),
+           cl::cat(ClangCheckCategory));
----------------
sammccall wrote:
> I think this should have a name with clear semantics like -analyzer-output-file (and only be used in analyzer mode).
> 
> While it does just append -o to the command-line, I think it will confuse users to treat it as a generic output flag.
> I think this should have a name with clear semantics like -analyzer-output-file (and only be used in analyzer mode).
> While it does just append -o to the command-line, I think it will confuse users to treat it as a generic output flag.

My original idea is to add a generic output flag to the `clang-check` tool, as it seems impossible for tool users to customize the output name. However, it will probably confuse users with the useless `-o` options set via `-extra-arg` or appended after `--` and with other `-fsyntax-only` actions, just as what you mentioned.

So, which you think is better? All with `-Xanalyzer` option
```
$ clang-check -analyze -Xanalyzer -analyzer-output-name=html_output -Xanalyzer -analyzer-output=html -Xanalyzer -analyzer-other-options=value input1.cc input2.cc ...
```
OR `-analyzer-output-name` without `-Xanalyzer` option
```
$  clang-check -analyze -analyzer-output-name=html_output -Xanalyzer -analyzer-output=html -Xanalyzer -analyzer-other-options=value input1.cc input2.cc ...
```
OR even more aggressively, only non-commonly used options with `-Xanalyzer` option
```
$  clang-check -analyze -analyzer-output-name=html_output -analyzer-output=html -Xanalyzer -analyzer-other-options=value input1.cc input2.cc ...
```


================
Comment at: clang/tools/clang-check/ClangCheck.cpp:213-221
+  Tool.appendArgumentsAdjuster(
+      [&](const CommandLineArguments &Args, StringRef File) {
+        auto Ret = getClangStripOutputAdjuster()(Args, File);
+        if (!Output.empty()) {
+          Ret.emplace_back("-o");
+          Ret.emplace_back(Output);
+        }
----------------
If we use the `-analyzer-output-name` option, we need to check whether `-analyze` is enabled and use the value to determine which adjuster should be used.



================
Comment at: clang/tools/clang-check/ClangCheck.cpp:215
+      [&](const CommandLineArguments &Args, StringRef File) {
+        auto Ret = getClangStripOutputAdjuster()(Args, File);
+        if (!Output.empty()) {
----------------
The original `-o` options will be first eliminated with the clang strip output adjuster in the lambda adjuster. Then, the new `-o` options will be appended if necessary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97265