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

Define a preprocessor flag for enabling warnings on unused ConnectionHandle #71

Merged
merged 4 commits into from
Jun 14, 2024

Conversation

phyBrackets
Copy link
Contributor

Resolving #43

src/kdbindings/signal.h Outdated Show resolved Hide resolved
Copy link
Contributor

@LeonMatthesKDAB LeonMatthesKDAB left a comment

Choose a reason for hiding this comment

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

Overall a good change.
The way it's currently implemented, it would make CMake required, which I would like to avoid.

That should easy to fix though.

src/kdbindings/CMakeLists.txt Outdated Show resolved Hide resolved
cmake/KDBindingsConfig.h.in Outdated Show resolved Hide resolved
@LeonMatthesKDAB LeonMatthesKDAB force-pushed the warn_for_unused_connectionHandle branch from f39723d to c0c23c8 Compare June 4, 2024 14:08
Copy link
Contributor Author

@phyBrackets phyBrackets left a comment

Choose a reason for hiding this comment

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

Thanks @LeonMatthesKDAB , Looks good to me now!

@LeonMatthesKDAB
Copy link
Contributor

@phyBrackets cool, can you please incorporate my last review, so we can merge the PR?

@phyBrackets phyBrackets force-pushed the warn_for_unused_connectionHandle branch from 420c52d to 6aa3585 Compare June 5, 2024 06:22
@LeonMatthesKDAB
Copy link
Contributor

LeonMatthesKDAB commented Jun 5, 2024

@phyBrackets Thanks for the update, the code is good now 👍

It just seems we have loads of unused ConnectionHandles in our tests 🤔
I guess we could use std::ignore to fix this.

It does bring up the question of how annoying this would be for end-users though.
@seanharmer If we enable this by default, do you think our existing user code would explode with warnings about unused ConnectionHandles?
Because then I may need to revert my opinion about making this ON by default.

If it's just our testing that discards connection handles that often, I'm fine with leaving it ON though.

@seanharmer
Copy link
Contributor

I'd suggest enabling it by default. It usually indicates something we're doing wrong so good to know and fix them.

@LeonMatthesKDAB
Copy link
Contributor

@seanharmer thanks for the feedback 👍 .

@phyBrackets can you please add the std::ignore calls where required, or use the ConnectionHandle if it makes sense, so CI passes.
Then we can merge the PR 🥳

Side note: To test locally, you should be able to just run cmake --preset=clazy && cmake --build --preset=clazy && ctest --preset=clazy, which enables all warnings.

@phyBrackets
Copy link
Contributor Author

@phyBrackets Thanks for the update, the code is good now 👍

It just seems we have loads of unused ConnectionHandles in our tests 🤔 I guess we could use std::ignore to fix this.

It does bring up the question of how annoying this would be for end-users though. @seanharmer If we enable this by default, do you think our existing user code would explode with warnings about unused ConnectionHandles? Because then I may need to revert my opinion about making this ON by default.

If it's just our testing that discards connection handles that often, I'm fine with leaving it ON though.

Hi @LeonMatthesKDAB , Thanks for the idea. I guess it would be fine with std::ignore as well but it seems more intended to be used with tuples and it is also not a recommended way by standard to be used for suppressing warning in this case as it could potentially break in future version of the spec. Casting to simply void seems fine but I am not sure about if it looks good for our examples? (fine for tests!)

@phyBrackets phyBrackets force-pushed the warn_for_unused_connectionHandle branch from 814644c to eeba48f Compare June 12, 2024 11:27
@LeonMatthesKDAB LeonMatthesKDAB force-pushed the warn_for_unused_connectionHandle branch from eeba48f to 93705ae Compare June 14, 2024 07:30
@LeonMatthesKDAB LeonMatthesKDAB merged commit 90a96ce into main Jun 14, 2024
5 checks passed
@LeonMatthesKDAB LeonMatthesKDAB deleted the warn_for_unused_connectionHandle branch August 28, 2024 06:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants