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

[fix] Resolve checker enable/disable ambiguity #4392

Merged
merged 1 commit into from
Dec 10, 2024

Conversation

cservakt
Copy link
Collaborator

This PR fixes the enabling and disabling checker options, focusing on resolving ambiguities in option processing.

Key updates:
Ambiguity handling:

When ambiguous checker options are provided with -e or -d flags, an error is now raised. The user receives suggestions for specifying the option by using namespaces (checker, prefix, guideline, etc.).
If a checker name matches multiple checkers as a prefix, it triggers an ambiguity error. Suggestions are provided to help users choose between the checker name or the prefix.

Additional namespaces:

A new checker namespace is created. All label types can be a namespace as well, specified with a : separator before the checker option.

Default profile settings improvements:

Only those checkers enabled that has profile:default label in the label files. Prefix-based configuration is no longer applied for the default profile, ensuring precise and predictable behavior.

Getting rid of W prefix:

It is no longer available to enable or disable a warrning with W.

@cservakt cservakt 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 Nov 29, 2024
@cservakt cservakt added this to the release 6.25.0 milestone Nov 29, 2024
@cservakt cservakt requested review from dkrupp and noraz31 November 29, 2024 16:25
@cservakt cservakt force-pushed the ambigous-checker-options branch 3 times, most recently from ee96319 to e428702 Compare December 3, 2024 12:10
@cservakt cservakt requested a review from bruntib December 3, 2024 12:11
Copy link
Member

@dkrupp dkrupp left a comment

Choose a reason for hiding this comment

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

  1. Please update the help of CodeChecker analyze --help and mention the :checker prefix option
  2. Update the analyzer user guide https://github.com/Ericsson/codechecker/blob/master/docs/analyzer/user_guide.md#toggling-checkers
    and remove any -Wwarning syntax suggestion. Describe the new "checker:" and "prefix:" syntax. Describe the name clash resolution logic

These are the direct remarks. More comments will come soon.

@cservakt cservakt force-pushed the ambigous-checker-options branch from e428702 to e6a8c5d Compare December 3, 2024 15:30
@cservakt cservakt requested a review from dkrupp December 3, 2024 15:31
Copy link
Member

@dkrupp dkrupp left a comment

Choose a reason for hiding this comment

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

Please also fix this section
https://github.com/Ericsson/codechecker/blob/master/docs/analyzer/user_guide.md#toggling-checkers
"
Command line --enable/--disable flags.
Their arguments may start with profile: of guideline: prefix which makes the choice explicit.
Without prefix it means a profile name, a guideline name or a checker group/name in this priority order."

Describe the checker:, prefix: and describe that priority order does not matter, an ambiguity causes error.

  1. remove all old --enable W* warning enable syntax from
    https://github.com/Ericsson/codechecker/blob/master/docs/analyzer/user_guide.md#toggling-compiler-warnings

section

docs/analyzer/user_guide.md Outdated Show resolved Hide resolved
@cservakt cservakt force-pushed the ambigous-checker-options branch 3 times, most recently from ba26d93 to eaf043f Compare December 9, 2024 14:40
This PR fixes the enabling and disabling checker options, focusing on resolving ambiguities in option processing.

Key updates:
	Ambiguity handling:
		When ambiguous checker options are provided with `-e` or `-d` flags, an error is now raised. The user receives suggestions for specifying the option by using namespaces (checker, prefix, guideline, etc.).
		If a checker name matches multiple checkers as a prefix, it triggers an ambiguity error. Suggestions are provided to help users choose between the checker name or the prefix.

	Additional namespaces:
		A new `checker` namespace is created. All label types can be a namespace as well, specified with a `:` separator before the checker option.

	Default profile settings improvements:
		Only those checkers enabled that has profile:default label in the label files. Prefix-based configuration is no longer applied for the default profile, ensuring precise and predictable behavior.

	Getting rid of `W` prefix:
		It is no longer available to enable or disable a warrning with W.
@cservakt cservakt force-pushed the ambigous-checker-options branch from eaf043f to fe19dc0 Compare December 10, 2024 06:24
Copy link
Member

@dkrupp dkrupp left a comment

Choose a reason for hiding this comment

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

Looks good.

I found 1 problem though:
CodeChecker analyze -e checker:nonextisting-checker

should give an error, but it accepts a non existing checker name.

@dkrupp dkrupp changed the title [fix] Enable and disable checkers [fix] Resolve checker enable/disable ambiguity Dec 10, 2024
@dkrupp dkrupp dismissed bruntib’s stale review December 10, 2024 12:29

All review comments were addressed.

@dkrupp dkrupp merged commit 4f14816 into Ericsson:master Dec 10, 2024
7 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analyzer 📈 Related to the analyze commands (analysis driver) bugfix 🔨 CLI 💻 Related to the command-line interface, such as the cmd, store, etc. commands
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants