move publisher and subscriber get_keyexpr implementation from impl.hxx to api.hxx #87
Conversation
…le declaration of get_keyexpr issue
|
I've added a very minimal example that is built in the github action (but possibly you don't want that as a part of CI feel free to change it or remove it), and there's another branch without the moved key_expr that demonstrates the issue: https://github.com/lucasw/zenoh-cpp/actions?query=branch%3Amultiple_declaration_ci++ - a specific example |
lucasw
force-pushed
the
multiple_get_keyexpr
branch
2 times, most recently
from
a10890d
to
f549284
Compare
lucasw
force-pushed
the
multiple_get_keyexpr
branch
from
1affa43
to
f9b78fd
Compare
|
Thanks @lucasw for your PR. We will have a look at it as soon as possible. |
There was a problem hiding this comment.
The fix for missing inline is very important and should be integrated before release. But change in example is not directly related to this missing inline fix and need to be adjusted to existing 'simple' example (btw I like the idea to rename them to 'standalone', this name better reflects the purpose)
I'm going to make separate PR with fix for inline issue only.
Later need to make systematic test for this issue: #55
There was a problem hiding this comment.
Not sure what's the purpose of this additional .cpp file? Also all other files in project have .cxx extension
There was a problem hiding this comment.
A, ok, I see, that's to test the linker error on multiple include. Anyway better to mention this explicitly and do this in tests instead of examples.
There was a problem hiding this comment.
This mostly repeats functionality of tests https://github.com/eclipse-zenoh/zenoh-cpp/tree/main/examples/simple
Though I like the name 'standalone' and agree that example in 'simple' can be made a bit more simple
I haven't tested much (
ctesttests all passed locally for me though) but this appears to solve the linker issue in eclipse-zenoh/roadmap#105All the other get_keyexpr() implementations were already in api.hxx and not impl.hxx, so this matches this pattern (but maybe not the intent of the api-impl split).