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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
