-
Notifications
You must be signed in to change notification settings - Fork 107
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
Suppress cppcheck unknownMacro #268
Conversation
cppcheck creates an unknownMacro error when it cannot resolve a macro. Since we don't pass in all dependent headers, we don't expect all macros to be discoverable by cppcheck. Signed-off-by: Dan Rose <dan@digilabs.io>
I think this will fully fix ros2/ros2#942 and obviates the need for ros2/rosidl_typesupport_connext#57, ros2/rosidl_typesupport_fastrtps#49, ros2/rosidl#505, ros2/rosidl_typesupport#78 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@dirk-thomas @jacobperron What do you think about using this approach instead of the one suggested in ros2/rclpy#577? |
I don't think this is a good approach since it removes a valid check and therefore reduces "coverage" of the linter. Not having the headers available which contain the macros also indicates that the linter can't do a complete / correct check. |
That's also true of excluding headers outside the project, which we do for performance reasons, which cripples the linter's ability to check for mistakes even more. I do think we should be adding ROS headers to the include path, but that the performance issues we're encountering with cppcheck are due to other misconfiguration of cppcheck, in particular not passing preprocessor definitions (since cppcheck checks every preprocessor branch when none is specified). |
I think that's true.
Not passing definitions might be a source of bad performance in
That's similar to the coverage we were having with cppcheck < 2.0, as this warning wasn't shown in those versions (it was shown sometimes, but a lot less frequently than with cppcheck >=2.0). |
Fast forward to ament/ament_lint#268 This suppresses a defect in `ament_cppcheck` - namely that if a macro is missing, it fails loudly. This is compounded by fact that `ament_cppcheck` does not include all dependent headers, so macros are likely to be missing in the first place.
Fast forward to ament/ament_lint#268 This suppresses a defect in `ament_cppcheck` - namely that if a macro is missing, it fails loudly. This is compounded by fact that `ament_cppcheck` does not include all dependent headers, so macros are likely to be missing in the first place. Signed-off-by: Dan Rose <dan@digilabs.io>
Fast forward to ament/ament_lint#268 This suppresses a defect in `ament_cppcheck` - namely that if a macro is missing, it fails loudly. This is compounded by fact that `ament_cppcheck` does not include all dependent headers, so macros are likely to be missing in the first place. Signed-off-by: Dan Rose <dan@digilabs.io>
FWIW, I tried to add headers of all dependencies back when we introduced passing headers to cppcheck in #117. The problem I ran into at the time was performance; it could take upwards of 300 seconds for cppcheck to run for some packages. It might be worth trying again to see if cppcheck performance has improved 🤷♂️ I agree that it would be preferable to not have to suppress another check for ROS core packages. |
Why did it take so long for some packages? If I suspect our root performance issue is the missing compile definitions, which I did my best to write up in #270
That argument cuts both ways. On one hand, you could be talking about suppressing But we do tolerate missing functions and missing classes, so tolerating missing macros doesn't seem like throwing out a lot of bathwater with this particular baby.
You should be able to consume upstream packages' interfaces without pulling in linter false positives. A lint error this fragile and confusing has no business being enabled by default. I think we can re-enable |
I second that. So, I think we should either figure out why it takes so long, or ignore the unknown macro warning. |
Fast forward to ament/ament_lint#268 This suppresses a defect in `ament_cppcheck` - namely that if a macro is missing, it fails loudly. This is compounded by fact that `ament_cppcheck` does not include all dependent headers, so macros are likely to be missing in the first place. Signed-off-by: Dan Rose <dan@digilabs.io>
Fast forward to ament/ament_lint#268 This suppresses a defect in `ament_cppcheck` - namely that if a macro is missing, it fails loudly. This is compounded by fact that `ament_cppcheck` does not include all dependent headers, so macros are likely to be missing in the first place. Signed-off-by: Dan Rose <dan@digilabs.io>
Fast forward to ament/ament_lint#268 This suppresses a defect in `ament_cppcheck` - namely that if a macro is missing, it fails loudly. This is compounded by fact that `ament_cppcheck` does not include all dependent headers, so macros are likely to be missing in the first place. Signed-off-by: Dan Rose <dan@digilabs.io>
What is the status of this PR? |
IMO, it's worth pursuing a solution that can automatically collect dependency headers and set the correct preprocessor values to cppcheck (as described in #270). As a workaround, I think individual projects can suppress the unknownMacro error either by passing preprocessor values (directly to cppcheck or by including headers via |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Newer versions of cppcheck (such as cppcheck-2.3, which is the current version in Fedora and EPEL) are triggering unknownMacro
exceptions far more often than cppcheck-1.90, which is the current version in Ubuntu Focal.
Until we can pursue a more programmatic solution like @jacobperron is describing, my vote is to suppress.
Can I think all C++ linters under |
With three approvals, I'll move forward with merging this change. If a patch lands for #270, we can try reverting this change. |
Linter tests only: Edit: both failures are due to hitting a timeout of 180 seconds, which was bumped on |
Yes, the only blocker was the increased test time (which was kind of unacceptable). |
cppcheck creates an unknownMacro error when it cannot resolve a macro.
Since we don't pass in all dependent headers, we don't expect all macros to be discoverable by cppcheck.