-
Notifications
You must be signed in to change notification settings - Fork 166
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
chore(libs): fix -Werror=unused-variable #2058
Conversation
Welcome @clan! It looks like this is your first PR to falcosecurity/libs 🎉 |
Perf diff from master - unit tests
Heap diff from master - unit tests
Heap diff from master - scap file
Benchmarks diff from master
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2058 +/- ##
=======================================
Coverage 73.55% 73.55%
=======================================
Files 253 253
Lines 31863 31863
Branches 5639 5631 -8
=======================================
Hits 23437 23437
- Misses 8410 8414 +4
+ Partials 16 12 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Thanks for this fix! Please note that CI issues are not caused by your PR; they are caused by #2053 that (correctly) enabled Also, i think your fix is valuable for next driver release! |
otherwise test will failed because of -Werror=unused-variable, then build will failed w/ kernel 6.4+ because of incorrect parameter of class_create patch has been submited to upstream, see falcosecurity/libs#2058 Signed-off-by: Z. Liu <[email protected]>
otherwise test will failed because of -Werror=unused-variable, then build will failed w/ kernel 6.4+ because of incorrect parameter of class_create patch has been submited to upstream, see falcosecurity/libs#2058 Signed-off-by: Z. Liu <[email protected]>
Hi! Can you rebase on latest master to fix the CI? :) thank you! |
otherwise test will give wrong result if compiler has -Werror flags, complained with: error: unused variable 'g_ppm_class' [-Werror=unused-variable] Signed-off-by: Z. Liu <[email protected]>
done. |
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.
/approve
LGTM label has been added. Git tree hash: 62b078793a34abef59dd3f3ca613bdeae86fd6e6
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: clan, FedeDP The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Out of curiosity, why does the cast fix this problem? It's side-effect-free and the compiler could still be allowed to delete everything except the return value. |
I think it's harmless for the feature test code, which won't cause run-time problem. I'm not sure whether there are any side effect (performance, etc.), may be depend on optimization level? What's the difference when compare to compiler's attribute (___attribute((unused)))? Will do more research when have time (by compare the generated asm code). |
I put it into CompilerExplorer here and as expected there is no difference between the cast and ((unused)). As soon as you enable |
What type of PR is this?
/kind bug
Any specific area of the project related to this PR?
/area build
Does this PR require a change in the driver versions?
No
What this PR does / why we need it:
otherwise test will give wrong result if compiler has -Werror flags, complained with:
error: unused variable 'g_ppm_class' [-Werror=unused-variable]
Does this PR introduce a user-facing change?:
No