search

jfjlaros / simpleRPC

Simple RPC implementation for Arduino.
MIT License
50 stars 17 forks source link

Allow pmf-conversion only for selected functions. #27

Closed slaff closed 2 years ago

slaff commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

Looks good, thank you.