Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CLI] Fix double clang-tidy config flags #3157

Merged
merged 1 commit into from
Jan 21, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions analyzer/codechecker_analyzer/analyzers/clangtidy/analyzer.py
Original file line number Diff line number Diff line change
Expand Up @@ -330,6 +330,27 @@ def construct_config_handler(cls, args, context):
if m.group('analyzer') == cls.ANALYZER_NAME:
analyzer_config[m.group('key')] = m.group('value')

# If both --analyzer-config and -config (in --tidyargs) is given then
# these need to be merged. Since "HeaderFilterRegex" has a default
# value in --analyzer-config, we take --tidyargs stronger so user can
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to add this information to the --analyzer-config / --tidyargs options help messages that the options in --tidyargs are stonger.

# overwrite its value.
for i, extra_arg in enumerate(handler.analyzer_extra_arguments):
if not extra_arg.startswith('-config'):
continue

# -config flag can be together or separate from its argument:
# "-config blabla" vs. "-config=blabla"
if extra_arg == '-config':
arg = handler.analyzer_extra_arguments[i + 1]
arg_num = 2
else:
arg = extra_arg[len('-config='):]
arg_num = 1

analyzer_config.update(json.loads(arg))
del handler.analyzer_extra_arguments[i:i + arg_num]
break

# TODO: This extra "isinsrance" check is needed for
# CodeChecker checkers --checker-config. This command also
# runs this function in order to construct a config handler.
Expand Down
5 changes: 4 additions & 1 deletion analyzer/codechecker_analyzer/cmd/analyze.py
Original file line number Diff line number Diff line change
Expand Up @@ -393,7 +393,10 @@ def add_arguments_to_parser(parser):
"default behaviour of this option you can "
"use the "
"'clang-tidy:take-config-from-directory="
"true' option.")
"true' option. If the file at --tidyargs "
"contains a -config flag then those "
"options extend these and override "
"\"HeaderFilterRegex\" if any.")

analyzer_opts.add_argument('--checker-config',
dest='checker_config',
Expand Down
5 changes: 4 additions & 1 deletion analyzer/codechecker_analyzer/cmd/check.py
Original file line number Diff line number Diff line change
Expand Up @@ -394,7 +394,10 @@ def add_arguments_to_parser(parser):
"default behaviour of this option you can "
"use the "
"'clang-tidy:take-config-from-directory="
"true' option.")
"true' option. If the file at --tidyargs "
"contains a -config flag then those "
"options extend these and override "
"\"HeaderFilterRegex\" if any.")

analyzer_opts.add_argument('--checker-config',
dest='checker_config',
Expand Down
16 changes: 10 additions & 6 deletions analyzer/tests/functional/analyze/test_analyze.py
Original file line number Diff line number Diff line change
Expand Up @@ -474,12 +474,12 @@ def test_tidyargs_saargs(self):
"""
build_json = os.path.join(self.test_workspace, "build_extra_args.json")
report_dir = os.path.join(self.test_workspace, "reports_extra_args")
source_file = os.path.join(self.test_dir, "extra_args.c")
source_file = os.path.join(self.test_dir, "extra_args.cpp")
tidyargs_file = os.path.join(self.test_dir, "tidyargs")
saargs_file = os.path.join(self.test_dir, "saargs")

build_log = [{"directory": self.test_dir,
"command": "cc -c " + source_file,
"command": "g++ -c " + source_file,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any test cases where you use the --analyzer-config and --tidyargs in the same command. So please create a new test case or extend an existing one with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I extended the content of analyzer/tests/functional/analyze/test_files/tidyargs file. Now it contains a -config flag. --analyzer-config also has some default value from earlier. The test is the fact that clang-tidy doesn't fail with these two provided and it also gives report on modernize- issues which is enabled in tidyargs.

Copy link
Contributor

@csordasmarton csordasmarton Jan 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I understand it but I would like to see a test for example where we set the clang-tidy:Checks and a clang-tidy:HeaderFilterRegex config option in the --analyzer-config option of the command line and in the --tidy-args I override for example the Checks option.

"file": source_file
}]

Expand All @@ -488,7 +488,9 @@ def test_tidyargs_saargs(self):
json.dump(build_log, outfile)

analyze_cmd = [self._codechecker_cmd, "analyze", build_json,
"-o", report_dir, "--tidyargs", tidyargs_file]
"-o", report_dir, "--tidyargs", tidyargs_file,
"--analyzer-config", 'clang-tidy:HeaderFilterRegex=.*',
'clang-tidy:Checks=modernize-use-bool-literals']

process = subprocess.Popen(
analyze_cmd,
Expand All @@ -509,6 +511,8 @@ def test_tidyargs_saargs(self):
out, _ = process.communicate()

self.assertIn("division by zero", out)
self.assertIn("modernize-avoid-bind", out)
self.assertNotIn("performance-for-range-copy", out)

analyze_cmd = [self._codechecker_cmd, "analyze", build_json,
"-o", report_dir, "--saargs", saargs_file]
Expand Down Expand Up @@ -821,13 +825,13 @@ def test_makefile_generation(self):
analyze_cmd = [self._codechecker_cmd, "analyze", build_json,
"-o", self.report_dir, '--makefile']

source_file = os.path.join(self.test_dir, "extra_args.c")
source_file = os.path.join(self.test_dir, "extra_args.cpp")
build_log = [{"directory": self.test_workspace,
"command": "gcc -DTIDYARGS -c " + source_file,
"command": "g++ -DTIDYARGS -c " + source_file,
"file": source_file
},
{"directory": self.test_workspace,
"command": "gcc -DSAARGS -DTIDYARGS -c " + source_file,
"command": "g++ -DSAARGS -DTIDYARGS -c " + source_file,
"file": source_file
}]

Expand Down
11 changes: 0 additions & 11 deletions analyzer/tests/functional/analyze/test_files/extra_args.c

This file was deleted.

20 changes: 20 additions & 0 deletions analyzer/tests/functional/analyze/test_files/extra_args.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
#include <functional>

int add(int x, int y) { return x + y; }

int main()
{
#ifdef TIDYARGS
int i = 1 / 0;
#endif

#ifdef SAARGS
int* p = 0;
*p = 42;
#endif

int x = 2;
auto clj = std::bind(add, x, std::placeholders::_1);

bool b = 1;
}
2 changes: 1 addition & 1 deletion analyzer/tests/functional/analyze/test_files/tidyargs
Original file line number Diff line number Diff line change
@@ -1 +1 @@
--extra-arg=-DTIDYARGS
--extra-arg=-DTIDYARGS '-config={"Checks": "modernize-avoid-bind"}'
32 changes: 18 additions & 14 deletions docs/analyzer/user_guide.md
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ optional arguments:
Set verbosity level.

log arguments:

Specify how the build information database should be obtained. You need to
specify either an already existing log file, or a build command which will be
used to generate a log file on the fly.
Expand Down Expand Up @@ -248,8 +248,10 @@ analyzer arguments:
options can be printed with 'CodeChecker analyzers
--analyzer-config'. To disable the default behaviour
of this option you can use the 'clang-tidy:take-
config-from-directory=true' option. (default: ['clang-
tidy:HeaderFilterRegex=.*'])
config-from-directory=true' option. If the file at
--tidyargs contains a -config flag then those options
extend these and override "HeaderFilterRegex" if any.
(default: ['clang-tidy:HeaderFilterRegex=.*'])
--checker-config [CHECKER_CONFIG [CHECKER_CONFIG ...]]
Checker configuration options in the following format:
analyzer:key=value. The collection of the options can
Expand All @@ -273,7 +275,7 @@ analyzer arguments:
solver only. (default: on)

cross translation unit analysis arguments:

These arguments are only available if the Clang Static Analyzer supports
Cross-TU analysis. By default, no CTU analysis is run when 'CodeChecker check'
is called.
Expand Down Expand Up @@ -307,7 +309,7 @@ cross translation unit analysis arguments:
second phase of the analysis. (default: parse-on-demand)

checker configuration:

Checkers
------------------------------------------------
The analyzer performs checks that are categorized into families or "checkers".
Expand All @@ -319,7 +321,7 @@ checker configuration:
'core.uninitialized' group. Please consult the manual for details. Disabling
certain checkers - such as the 'core' group - is unsupported by the LLVM/Clang
community, and thus discouraged.

Compiler warnings and errors
------------------------------------------------
Compiler warnings are diagnostic messages that report constructions that are
Expand Down Expand Up @@ -899,13 +901,13 @@ one explicitly specified to **be analyzed** (`/dir/do.check.this.file`).
```

In the above example, `important_file.cpp` will be analyzed even if every file
where the path matches to `/my_project/my_lib_to_skip` will be skiped.
Every other file where the path contains `/myproject` except the files in the
where the path matches to `/my_project/my_lib_to_skip` will be skiped.
Every other file where the path contains `/myproject` except the files in the
`my_project/3pplib` will be analyzed.

The provided *shell-style* pattern is converted to a regex with the [fnmatch.translate](https://docs.python.org/2/library/fnmatch.html#fnmatch.translate).

Please note that when `-i SKIPFILE` is used along with `--stats` or
Please note that when `-i SKIPFILE` is used along with `--stats` or
`--ctu` the skip list will be ignored in the pre-analysis phase. This means
that statistics and ctu-pre-analysis will be created for *all* files in the
*compilation database*.
Expand All @@ -919,7 +921,7 @@ analyzer arguments:
Currently supported analyzers are: clangsa, clang-
tidy.
--add-compiler-defaults
DEPRECATED. Always True.
DEPRECATED. Always True.
Retrieve compiler-specific configuration from the
compilers themselves, and use them with Clang. This is
used when the compiler on the system is special, e.g.
Expand Down Expand Up @@ -961,8 +963,10 @@ analyzer arguments:
options can be printed with 'CodeChecker analyzers
--analyzer-config'. To disable the default behaviour
of this option you can use the 'clang-tidy:take-
config-from-directory=true' option. (default: ['clang-
tidy:HeaderFilterRegex=.*'])
config-from-directory=true' option. If the file at
--tidyargs contains a -config flag then those options
extend these and override "HeaderFilterRegex" if any.
(default: ['clang-tidy:HeaderFilterRegex=.*'])
--checker-config [CHECKER_CONFIG [CHECKER_CONFIG ...]]
Checker configuration options in the following format:
analyzer:key=value. The collection of the options can
Expand Down Expand Up @@ -1034,7 +1038,7 @@ Lets assume you have a configuration file
```
This configuration file example contains configuration options for multiple
codechecker subcommands (analyze, parse, server, store) so not just the
`analyze` subcommand can be configured like this.
`analyze` subcommand can be configured like this.
The focus is on the `analyze` subcommand configuration in the next examples.

If you run the following command:
Expand Down Expand Up @@ -1469,7 +1473,7 @@ Statistics analysis feature arguments:
function (calculated as calls with a property/all
calls). CodeChecker will warn for calls of f that do
not have that property.(default: 0.85)

```

## `parse` <a name="parse"></a>
Expand Down