[bugfix] Required format for --checker-config #3817

bruntib · 2023-01-16T10:29:40Z

"CodeChecker analyze" command has a --checker-config flag. The parameter
of this flag should be in the following format:
<analyzer>:<checker>:<option>=<value>

clang-tidy tool requires a "." character between the checker and the
option, so CodeChecker should convert it from : to .

The format of --checker-config (and --analyzer-config) is now checked
at the command line parsing phase, so it is done with a common algorithm
for all analyzer tools.

Earlier the default value of --analyzer-config was
"clang-tidy:HeaderFilterRegex='.*'", so clang-tidy analysis of header
files is also done. This is not a default value for the flag anymore,
but it is handled under clang-tidy module.

bruntib · 2023-01-16T10:38:19Z

Please don't review it yet. I'd rather like to give a whole different implementation for this issue.

bruntib · 2023-01-20T15:40:20Z

The alternative implementation is done, it can be reviewed.

Szelethus

LGTM, I guess! I like that the code in most places got a lot simpler.

Szelethus · 2023-01-24T12:18:13Z

analyzer/codechecker_analyzer/analyzers/cppcheck/analyzer.py

@@ -7,6 +7,7 @@
 # -------------------------------------------------------------------------
 """
 """


Might as well remove these?

I rather added some minimal documentation in order to keep consistency.

Szelethus

My usual gripe -- please, cope the commit message to the description.

Szelethus · 2023-02-03T12:54:45Z

analyzer/codechecker_analyzer/arg.py

+
+    if not m:
+        raise argparse.ArgumentTypeError(
+            f"Checker option in wrong format: {arg}")


...should be

Suggested change

f"Checker option in wrong format: {arg}")

f"Checker option in wrong format: {arg}, should be <analyzer>:<checker>:<option>=<value>")

vodorok

Please see my remarks

vodorok · 2023-02-06T16:28:46Z

analyzer/codechecker_analyzer/analyzers/clangtidy/analyzer.py

+                if cfg.analyzer == cls.ANALYZER_NAME:
+                    analyzer_config[cfg.option] = cfg.value
+
+        if not analyzer_config:


Can you explain in a comment why we adding this minimal configuration?

vodorok · 2023-02-06T16:34:40Z

analyzer/codechecker_analyzer/analyzers/clangtidy/analyzer.py

@@ -6,6 +6,7 @@
 #
 # -------------------------------------------------------------------------
 """
+ClangTidy related functions.


This is a very shy module comment.

vodorok · 2023-02-06T16:43:18Z

analyzer/codechecker_analyzer/arg.py

@@ -87,3 +89,47 @@ def existing_abspath(path: str) -> str:
        raise argparse.ArgumentTypeError(f"File doesn't exist: {path}")

    return path
+
+
+def analyzer_config(arg: str):


Can you specifiy the return type, if you define the NamedTuple at the module level?

vodorok · 2023-02-06T16:43:44Z

analyzer/codechecker_analyzer/arg.py

+        m.group("analyzer"), m.group("option"), m.group("value"))
+
+
+def checker_config(arg: str):


Can you specifiy the return type, if you define the NamedTuple at the module level?

vodorok · 2023-02-06T16:56:28Z

analyzer/codechecker_analyzer/arg.py

+    if not m:
+        raise argparse.ArgumentTypeError(
+            f"Checker option in wrong format: {arg}, should be"
+            "<analyzer>:<checker>:<option>=<value>")


These error messages, and the examples provided in the help messages should be in the same format. I prefer these ... variant, so please modify the help messages of argparser. (You can chose the other, if you want.)

Could you please give an example?

vodorok · 2023-02-06T17:03:12Z

analyzer/tests/functional/cmdline/test_cmdline.py

+        return_code, _, err = run_cmd(cmd)
+
+        self.assertEqual(return_code, 1)
+        self.assertIn("Checker option in wrong format", err)


Can you make a negative testcase for testing a mistyped analyzer option too?

vodorok · 2023-02-06T17:06:01Z

docs/analyzer/user_guide.md

-                        of the clang-tidy binary. (default: ['clang-
-                        tidy:HeaderFilterRegex=.*'])
+                        a -config flag then those options extend these. To use
+                        analyzer configuration file in case of Clang Tidy


Nitpick, but for my ears this rings better: ...To use an analyzer configuration...

vodorok · 2023-02-06T17:08:08Z

docs/analyzer/user_guide.md

-                        option. It will skip setting the '-checks' parameter
-                        of the clang-tidy binary. (default: ['clang-
-                        tidy:HeaderFilterRegex=.*'])
+                        a -config flag then those options extend these. To use


Nitpick, but for my ears this rings better: ...To use an analyzer configuration...

"CodeChecker analyze" command has a --checker-config flag. The parameter of this flag should be in the following format: <analyzer>:<checker>:<option>=<value> clang-tidy tool requires a "." character between the checker and the option, so CodeChecker should convert it from : to . The format of --checker-config (and --analyzer-config) is now checked at the command line parsing phase, so it is done with a common algorithm for all analyzer tools. Earlier the default value of --analyzer-config was "clang-tidy:HeaderFilterRegex='.*'", so clang-tidy analysis of header files is also done. This is not a default value for the flag anymore, but it is handled under clang-tidy module.

vodorok

LGTM

Szelethus

LGTM

bruntib added CLI 💻 Related to the command-line interface, such as the cmd, store, etc. commands bugfix 🔨 analyzer 📈 Related to the analyze commands (analysis driver) labels Jan 16, 2023

bruntib added this to the release 6.22.0 milestone Jan 16, 2023

bruntib requested a review from Szelethus January 16, 2023 10:29

bruntib requested a review from vodorok as a code owner January 16, 2023 10:29

bruntib added the WIP 💣 Work In Progress label Jan 16, 2023

bruntib force-pushed the tidy_config_format branch from af52f5e to fe3c6c3 Compare January 20, 2023 15:39

bruntib requested a review from dkrupp as a code owner January 20, 2023 15:39

bruntib removed the WIP 💣 Work In Progress label Jan 20, 2023

Szelethus approved these changes Jan 24, 2023

View reviewed changes

bruntib force-pushed the tidy_config_format branch from fe3c6c3 to 95e4f46 Compare January 24, 2023 16:54

Szelethus requested changes Feb 3, 2023

View reviewed changes

bruntib force-pushed the tidy_config_format branch from 95e4f46 to 9675a43 Compare February 6, 2023 13:47

vodorok requested changes Feb 6, 2023

View reviewed changes

bruntib mentioned this pull request Feb 23, 2023

[refactor] Reducing analyzer config handler #3824

Merged

bruntib force-pushed the tidy_config_format branch from 9675a43 to 1a7a53e Compare February 24, 2023 10:18

bruntib requested review from vodorok and Szelethus March 20, 2023 12:49

vodorok approved these changes Mar 24, 2023

View reviewed changes

Szelethus approved these changes Mar 24, 2023

View reviewed changes

bruntib merged commit 6f92895 into Ericsson:master Mar 24, 2023

bruntib deleted the tidy_config_format branch March 24, 2023 14:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[bugfix] Required format for --checker-config #3817

[bugfix] Required format for --checker-config #3817

bruntib commented Jan 16, 2023 •

edited

Loading

bruntib commented Jan 16, 2023

bruntib commented Jan 20, 2023

Szelethus left a comment

Szelethus Jan 24, 2023

bruntib Jan 24, 2023

Szelethus left a comment

Szelethus Feb 3, 2023

vodorok left a comment •

edited

Loading

vodorok Feb 6, 2023

vodorok Feb 6, 2023

vodorok Feb 6, 2023

vodorok Feb 6, 2023

vodorok Feb 6, 2023

bruntib Feb 24, 2023

vodorok Feb 6, 2023

vodorok Feb 6, 2023

vodorok Feb 6, 2023

vodorok left a comment

Szelethus left a comment

	f"Checker option in wrong format: {arg}")
	f"Checker option in wrong format: {arg}, should be <analyzer>:<checker>:<option>=<value>")

		m.group("analyzer"), m.group("option"), m.group("value"))


		def checker_config(arg: str):

[bugfix] Required format for --checker-config #3817

[bugfix] Required format for --checker-config #3817

Conversation

bruntib commented Jan 16, 2023 • edited Loading

bruntib commented Jan 16, 2023

bruntib commented Jan 20, 2023

Szelethus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Szelethus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vodorok left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vodorok left a comment

Choose a reason for hiding this comment

Szelethus left a comment

Choose a reason for hiding this comment

bruntib commented Jan 16, 2023 •

edited

Loading

vodorok left a comment •

edited

Loading