[CLI] Fix double clang-tidy config flags #3157
Conversation
bruntib
added
CLI 💻
bruntib
force-pushed
the
clang_tidy_double_config
branch
from
5a578cd to
ffefdba
Compare
| @@ -330,6 +330,23 @@ 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 | |||
There was a problem hiding this comment.
It would be good to add this information to the --analyzer-config / --tidyargs options help messages that the options in --tidyargs are stonger.
| 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, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| if extra_arg.startswith('-config'): | ||
| 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 |
There was a problem hiding this comment.
In python I always like to save indentation where it is possible. Here we can do the following thing:
| if extra_arg.startswith('-config'): | |
| 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 | |
| if not extra_arg.startswith('-config'): | |
| continue | |
| 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 |
| # overwrite its value. | ||
| for i, extra_arg in enumerate(handler.analyzer_extra_arguments): | ||
| if extra_arg.startswith('-config'): | ||
| if extra_arg == '-config': |
There was a problem hiding this comment.
An extra comment could be useful here why do we need this condition.
bruntib
force-pushed
the
clang_tidy_double_config
branch
from
ffefdba to
6750e99
Compare
`clang-tidy` can be given config options via --analyzer-config and -config in --tidyargs. --analyzer-config also has a default value, so in case --tidyargs also contains a -config flag then both is given to clang-tidy which thus fails.
bruntib
force-pushed
the
clang_tidy_double_config
branch
from
6750e99 to
50b2a41
Compare
clang-tidycan be given config options via --analyzer-config and-config in --tidyargs. --analyzer-config also has a default value, so in
case --tidyargs also contains a -config flag then both is given to
clang-tidy which thus fails.