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

Allow pmf-conversion only for selected functions. #27

Merged

Conversation

slaff
Copy link
Collaborator

@slaff slaff commented May 4, 2022

This PR allows the usage of simpleRPC without having to explicitly provide a pmf-conversion flag during compilation.

More about pmf-conversions: https://gcc.gnu.org/onlinedocs/gcc/Bound-member-functions.html
More about disabling compile-type warnings in GCC and Clang: https://www.fluentcpp.com/2019/08/30/how-to-disable-a-warning-in-cpp/.

@jfjlaros
Copy link
Owner

jfjlaros commented May 4, 2022

Is it also possible to only add this one line to simpleRPC.h?

#pragma GCC diagnostic ignored "-Wpmf-conversions"

For example on line 3.

@slaff
Copy link
Collaborator Author

slaff commented May 5, 2022

If it's added in simpleRPC.h then this warning suppression will be valid for every C++ code where simpleRPC.h gets included. Which might hide actual issues. Putting the suppression only around the functions where the PMF version is expected and should be allowed will get us the best of both worlds - support for PMF in the code that is using it and detection of PMF issue in all other places that are including simpleRPC.h. The suppression and detection is done during compile time from the compiler.

@jfjlaros
Copy link
Owner

jfjlaros commented May 5, 2022

That is true.

What about inserting the following before line 3

#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wpmf-conversions"

and this before line 9?

#pragma GCC diagnostic pop

I know that it is not as specific as what you suggest, but it would be a bit tidier.

And I guess we can remove -Wno-pmf-conversions from line 9 of tests/Makefile.

@slaff
Copy link
Collaborator Author

slaff commented May 5, 2022

And I guess we can remove -Wno-pmf-conversions from line 9 of tests/Makefile.

Yes, correct.

I know that it is not as specific as what you suggest, but it would be a bit tidier.

Should work. Ok, I will update the source code.

@slaff
Copy link
Collaborator Author

slaff commented May 5, 2022

You can see the difference in the initial suggestion (Expand "Test with Catch") and the newer version. With the new changes we kinda of hope that the developers will never include rcpCall.tcc and signature.tcc directly.

@jfjlaros
Copy link
Owner

jfjlaros commented May 5, 2022

Hmm, perhaps we should leave the Makefile as is then.

As for including rpcCall.tcc directly, I presume that people that want to do that know what they are doing.

By the way, you can add your name to docs/credits.rst if you want.

@jfjlaros
Copy link
Owner

jfjlaros commented May 5, 2022

Looks good, thank you.

@jfjlaros jfjlaros merged commit 947d7f7 into jfjlaros:master May 5, 2022
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.

None yet

3 participants