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

[bugfix] Allow the disabling of statisticsbased checkers #3972

Merged
merged 1 commit into from
Sep 1, 2023

Conversation

bruntib
Copy link
Contributor

@bruntib bruntib commented Aug 4, 2023

When --stats flag is given to "CodeChecker analyze" these two statistics-based checkers are enabled by default:

  • statisticsbased.SpecialReturnValue
  • statisticsbased.UncheckedReturnValue

These can't be disabled even with -d/--disable if they were enabled by --stats.

This patch fixes this bug: now users can disable either statistics-based checker, even if --stats is used.

@bruntib bruntib added CLI 💻 Related to the command-line interface, such as the cmd, store, etc. commands bugfix 🔨 labels Aug 4, 2023
@bruntib bruntib added this to the release 6.23.0 milestone Aug 4, 2023
@bruntib bruntib requested a review from Szelethus August 4, 2023 14:26
@bruntib bruntib requested a review from vodorok as a code owner August 4, 2023 14:26
@Szelethus Szelethus changed the title [bugfix] Allow statisticsbased checker disable [bugfix] Allow the disabling of statisticsbased checkers Aug 7, 2023
Copy link
Contributor

@Szelethus Szelethus left a comment

Choose a reason for hiding this comment

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

Seems like you don't actually test whether you can disable those checkers if --stats is set. On another note, shouldn't we want if --stats is enabled but both statistics checkers are explicitly disabled?

@@ -94,6 +98,19 @@ def parse_clang_help_page(
return res


def _is_user_disabled_checker(checker, ordered_checkers):
"""
This function returns True if the given statistics-based checker is
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like this is not a statisticsbased specific function. If it is important for those checkers specifically, document it why. I'd expect to see something like

"Since these checkers can be enabled through -e/--enable AND --stats, we need special handling for them to make sure -d/--disable overwrite --stats"

if 'stats_enabled' in args:
# In some Clang versions these checkers may be in some checker
# group (e.g. alpha...), so let's find the actual checker name
# first.
Copy link
Contributor

Choose a reason for hiding this comment

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

I see what you wanted to say, but its a little sloppy. How about this:

"In some Clang versions, statisticsbased checkers are in the alpha package (alpha.statisticsbased), and maybe not in others. Lets figure out the actual checker name first"

handler.set_checker_enabled(unchecked_return_value)

# Statistics collector checkers must be explicitly disabled
# as they trash the output.
Copy link
Contributor

Choose a reason for hiding this comment

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

You should also state that their job was done anyway in a preceding analysis phase. Otherwise I enjoy the sh!ttalk :)

@bruntib bruntib force-pushed the stat_checker_disable branch from 5175e8c to 70cae53 Compare August 21, 2023 08:13
@bruntib bruntib requested a review from Szelethus August 21, 2023 08:13
Copy link
Contributor

@Szelethus Szelethus left a comment

Choose a reason for hiding this comment

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

Please address the nits. Otherwise LGTM

handler.set_checker_enabled(unchecked_return_value)

# Statistics collector checkers must be explicitly disabled as they
# trash the output. Thise checkers are executed in a preceding analysis
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# trash the output. Thise checkers are executed in a preceding analysis
# trash the output. These checkers are executed in a preceding analysis

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, almost there, how about you end the sentence with "so they are not needed here."

When --stats flag is given to then command "CodeChecker analyze" then
two statistics-based checkers are enabled by default:

- statisticsbased.SpecialReturnValue
- statisticsbased.UncheckedReturnValue

These can't be disabled if --stats flag is used.

This patch fixes this bug: now users can disable either statistics-based
checker, even if --stats is used.
@bruntib bruntib force-pushed the stat_checker_disable branch from 70cae53 to ed5c0be Compare September 1, 2023 08:51
@bruntib bruntib merged commit f912cf0 into Ericsson:master Sep 1, 2023
@bruntib bruntib deleted the stat_checker_disable branch September 1, 2023 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

2 participants