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.

Sign up for GitHub

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] Treat clang-diagnostic-* checkers as compiler flags #3874

Merged
merged 5 commits into from
Apr 14, 2023
Merged

[fix] Treat clang-diagnostic-* checkers as compiler flags #3874

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
533b3c3
[fix] Treat clang-diagnostic-* checkers as compiler flags
vodorok Apr 12, 2023
cf6fbf1
Try a different approach
vodorok Apr 12, 2023
7dfd1ee
Fix --enable-all mode, and convert tests
vodorok Apr 12, 2023
c5f4858
Fix web tests, and comments
vodorok Apr 12, 2023
c1f1bbb
Refactor warning determination logic
vodorok Apr 13, 2023
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
34 changes: 25 additions & 9 deletions analyzer/codechecker_analyzer/analyzers/clangtidy/analyzer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,9 @@ def get_checker_list(self, config) -> Tuple[List[str], List[str]]:
some ClangSA bug.
"""
checkers = []
compiler_warnings = []
# Usage of a set will remove compiler warnings and clang-diagnostics
# which are the same.
compiler_warnings = set()

has_checker_config = \
config.checker_config and config.checker_config != '{}'
Expand All @@ -227,16 +229,30 @@ def get_checker_list(self, config) -> Tuple[List[str], List[str]]:
state, _ = value

# Checker name is a compiler warning.
if checker_name.startswith('W'):
Szelethus marked this conversation as resolved.
Show resolved Hide resolved
LOG.warning(f"The following usage of {checker_name} "
"compiler warning as a checker name is "
"deprecated, please use "
"clang-diagnostic-<checker-name> instead")
vodorok marked this conversation as resolved.
Show resolved Hide resolved

warning_name = get_compiler_warning_name(checker_name)
if warning_name is not None:
# -W and clang-diagnostic- are added as compiler warnings.
if state == CheckerState.enabled:
compiler_warnings.append('-W' + warning_name)
compiler_warnings.add('-W' + warning_name)
elif state == CheckerState.disabled:
compiler_warnings.append('-Wno-' + warning_name)

compiler_warnings.add('-Wno-' + warning_name)
continue
elif checker_name.startswith('clang-diagnostic'):
checker_name += '*'

# If clang-diagnostic is enabled, else add it as a disabled
# check.
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean to say that all clang-diagnostics not explicitly enabled should be disabled?

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'll rephrase this comment. I wanted to write something like this:

If a clang-diagnostic is enabled add it as a -W flag, else add it as a disabled checker to the checkers list.

Copy link
Contributor

Choose a reason for hiding this comment

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

This english is still a little broken.

if checker_name.startswith('clang-diagnostic-') and \
Copy link
Contributor

Choose a reason for hiding this comment

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

We already have the variable warning_name, that holds the information on whether we recognized the checker as compiler warning, no? Why are we reinveting the wheel instead of reusing the result of get_compiler_warning_name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At the first iteration of this pr, I modified get_compiler_warning_name in a manner that it worked on clang-diagnostic- checker names. I did not like this approach, because in that implementation all the disabled clang-diagnostic- checkers were included as -Wno compiler flags in the build command passed to tidy.

Copy link
Contributor

Choose a reason for hiding this comment

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

I still think this is bad design. The code would look more readable if we moved that logic to get_compiler_warning_name (where it belongs), and we really need to, check whether this is a disabled clang-diagnostic after the fact.

state == CheckerState.enabled:
warning_name = checker_name[len('clang-diagnostic-'):]
if state == CheckerState.enabled:
compiler_warnings.add('-W' + warning_name)
elif state == CheckerState.disabled:
checkers.append('-' + checker_name)

if state == CheckerState.enabled:
checkers.append(checker_name)
Expand All @@ -250,7 +266,7 @@ def get_checker_list(self, config) -> Tuple[List[str], List[str]]:
# for the compiler, clang-tidy is just capable to emit them in its own
# format.
if config.analyzer_config.get('take-config-from-directory') == 'true':
return [], compiler_warnings
return [], list(compiler_warnings)

if has_checker_config:
try:
Expand All @@ -263,12 +279,12 @@ def get_checker_list(self, config) -> Tuple[List[str], List[str]]:
# valid dictionary.
checker_cfg = ast.literal_eval(config.checker_config.strip())
if checker_cfg.get('Checks'):
return [], compiler_warnings
return [], list(compiler_warnings)
except SyntaxError as ex:
LOG.debug("Invalid checker configuration: %s. Error: %s",
config.checker_config, ex)

return checkers, compiler_warnings
return checkers, list(compiler_warnings)

def construct_analyzer_cmd(self, result_handler):
""" Contruct command which will be executed on analysis. """
Expand Down
5 changes: 3 additions & 2 deletions analyzer/codechecker_analyzer/analyzers/config_handler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,9 @@ class CheckerState(Enum):

def get_compiler_warning_name(checker_name):
"""
Removes 'W' or 'Wno' from the compiler warning name, if this is a
compiler warning. Returns None otherwise.
Removes 'W' or 'Wno' from the compiler warning name, or
'clang-diagnostic-' from the checker name.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is just not true right now.

Returns None otherwise.
"""
# Checker name is a compiler warning.
if checker_name.startswith('W'):
Expand Down
27 changes: 27 additions & 0 deletions analyzer/tests/unit/test_checker_handling.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -316,3 +316,30 @@ def test_only_clangsa_analyzer_checks_are_disabled(self):
self.assertFalse(
any(check.startswith('-') and check != '-clang-analyzer-*'
for check in self.__class__.checks_list))

def test_clang_diags_as_compiler_warnings(self):
"""
Test that clang-diagnostic-* checkers are enabled as compiler warnings.
"""

args = Namespace()
args.ordered_checkers = [
# This should enable -Wvla and -Wvla-extension.
('clang-diagnostic-vla', True),
('clang-diagnostic-unused-value', False)
]
analyzer = create_analyzer_tidy(args)
result_handler = create_result_handler(analyzer)

analyzer.config_handler.checker_config = '{}'
analyzer.config_handler.analyzer_config = \
{'take-config-from-directory': 'true'}

cmd = analyzer.construct_analyzer_cmd(result_handler)

# We expect that the clang-diagnostic-vla
# and clang-diagnostic-vla-extension is enabled as -Wvla and
# -Wvla-extension. The clang-diagnostic-unused-value is disabled as
# -Wno-unused-value.
self.assertEqual(cmd.count('-Wvla'), 1)
self.assertEqual(cmd.count('-Wvla-extension'), 1)