search

ros2 / rclc

ROS Client Library for the C language.
Apache License 2.0
118 stars 42 forks source link

Better executor type names #378

Closed JanStaschulat closed 1 year ago

JanStaschulat commented 1 year ago 
          Having recently updated this package in my local build env to include this PR, I started running into build errors.

One of my platform headers already #defines a NONE, leading to problems with the value in the rclc_executor_type_t enum (here).

While I would agree with a statement along the lines of "that's not a very good idea to globally #define something called NONE", unfortunately I can't change my platform's header.

Would it perhaps be possible to prefix the names in the enum in executor.h, such that the chances of clashing with existing/platform #defines are reduced? Perhaps an EXECUTOR_ prefix?

Edit: the LET member in rclc_executor_semantics_t is also perhaps a candidate for a prefix, although I haven't yet ran into an issue with that.

Originally posted by @gavanderhoorn in https://github.com/ros2/rclc/issues/355#issuecomment-1605652975

gavanderhoorn commented 1 year ago

The suggested NON_INITIALIZED_EXECUTOR (for NONE) and LOGICAL_EXECUTION_TIME (for LET) would solve the immediate issue I have (ie: the name clash).

As I wrote in https://github.com/ros2/rclc/pull/355#issuecomment-1607218381, some of the other members (SINGLE_THREADED and MULTI_THREADED) are rather generically named as well.

Seeing as there is no enum scope in C11, prefixing those might also be a good idea.

gavanderhoorn commented 1 year ago

I'll close this with #379 and #382 merged.

gavanderhoorn commented 1 year ago

O wait, it's your issue @JanStaschulat. Can't close it.