Conversation
|
CI (everything, just to ensure there is no higher-level breakage):
|
wjwwood
left a comment
There was a problem hiding this comment.
I would actually prefer to see this pattern:
typedef struct rcutils_foo_s {
// ...
} rcutils_foo_t;My reasons for this:
- we already have code like
rcutils_foo_t rcutils_foo = ...; - other users (outside our code base) could be using
rcutils_foo(without the_t) causing a collision, but I think it is much less likely anyone is usingrcutils_foo_s - having some suffix helps denote that it may be a complex type
- though it is discouraged to use
_tin your projects, the reasoning is thatPOSIXmay arbitrarily add new<type>_ttypes, but since we namespace 100% of our C symbols with<package_name>_, that's not a problem - when using auto-complete seeing
foo_tandfoo_sindicates to me (without knowing the code base) which is the struct and which is the typedef, i.e. withfooandfoo_tit could just as easily bestruct foo_tasstruct fooin my opinion
Thoughts?
If others agree, then I can update this pull request and add some documentation to our contributing/style guides.
I should clarify I think this is the case if we expand this pattern to other packages like |
Currently we use something like:
typedef struct foo_t
{
} foo_t;
But that actually double-defines the `foo_t` symbol
(at least, according to Doxygen). Instead, use the
more traditional:
typedef struct foo_s
{
} foo_t;
This is slightly controversial in that it is an API break;
any code that was using `struct foo_t` before will now fail
to compile (anything that was just using `foo_t` will be fine).
However, this should mostly affect low-level libraries
like client libraries and the such, so the scale of breakage
here should be low.
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
I honestly don't have a strong opinion either way, so I'll go ahead and rebase this and change it to |
a30f1b9 to
415ebda
Compare
|
CI:
|
|
Rerunning CI since ros2/system_tests#481 and ros2/rcl_interfaces#125 were merged:
|
|
The failures in Windows were also in the nightlies. I'm going to go ahead and merge this, thanks for the reviews! |
|
This pull request has been mentioned on ROS Discourse. There might be relevant details there: https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-2021-9-16/22372/1 |
Currently we use something like:
But that actually double-defines the
foo_tsymbol(at least, according to Doxygen). Instead, use the
more traditional:
This is slightly controversial in that it is an API break;
any code that was using
struct foo_tbefore will now failto compile (anything that was just using
foo_twill be fine).However, this should mostly affect low-level libraries
like client libraries and the such, so the scale of breakage
here should be low.
Signed-off-by: Chris Lalancette clalancette@openrobotics.org