-
Notifications
You must be signed in to change notification settings - Fork 54
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
[WIP] Add mimick based patch() functionality. #92
Conversation
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
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.
I think the PR looks pretty decent, but some more unit test examples illustrating its usage would be helpful.
[](rcutils_allocator_t allocator) { | ||
return rcutils_strdup("dummy_name", allocator); | ||
}); | ||
EXPECT_EQ("dummy_name", rcpputils::get_executable_name()); |
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.
EXPECT_STREQ or EXPECT_EQ?
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.
EXPECT_EQ
, as rcpputils::get_executable_name()
returns an std::string
.
|
||
} // namespace rcpputils | ||
|
||
#define RCPPUTILS_PATCH_IGNORE_OPERATOR(type, op) \ |
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.
I'm sure you'll add more info in comments, but what purpose does overloading these operators have? It's not clear from the macro name.
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.
For every argument type, a set of comparison operators must be defined (even if meaningless) or otherwise mimick
's macro generated code won't compile. This macro attempts to be a handy way to generate these dummy overloads. Name is up for debate 😅
FWIW I tried to come up with a smarter mechanism to hide this but couldn't find a way.
{ | ||
|
||
template<size_t N, typename SignatureT> | ||
struct patch_traits; |
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.
For these structs, c++ style suggests using PascalCase yah?
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.
Fair enough. I tend to fallback to stl
style whenever writing templates.
#define RCPPUTILS_PATCH_TARGET(what, where) \ | ||
(std::string(RCUTILS_STRINGIFY(where)) + "@" + (what)) | ||
|
||
#define patch(what, where, with) \ |
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.
Your macro name is the same as your struct name, will that lead to confusion?
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.
Perhaps. I don't expect the base class being used directly, but we can rename it.
Based on ros2/rcutils#272 (comment), making |
Closing this PR. |
Spin off from ros2/rcutils#272. Still lacking documentation, opening for early review.