Deprecate PointCloud message and APIs using it #107
Conversation
| @@ -71,6 +75,7 @@ static inline int getPointCloud2FieldIndex( | |||
| * \param input the message in the sensor_msgs::msg::PointCloud format | |||
| * \param output the resultant message in the sensor_msgs::msg::PointCloud2 format | |||
| */ | |||
| RCUTILS_DEPRECATED_WITH_MSG("PointCloud is deprecated as of Foxy to be removed after. Please use sensor_msgs/PointCloud2.") | |||
There was a problem hiding this comment.
C++14 has already a deprecated attribute: https://en.cppreference.com/w/cpp/language/attributes/deprecated.
Except the code here requires pre-C++14 compatibility, I would limits the usage of the new macros to pure C code.
tfoote
force-pushed
the
tfoote/deprecate_point_cloud
branch
from
4c27ca5 to
84dc9a0
Compare
|
Package only builds:
For good measure here's a sanity check with a full downstream rebuild.
|
| #ifdef _MSC_VER | ||
| #pragma message(POINT_CLOUD_DEPRECATION_MESSAGE) | ||
| #else | ||
| #ifndef RCUTILS_SKIP_WARNING |
There was a problem hiding this comment.
shoudn't RCUTILS_SKIP_WARNING also be taken in account on Windows?
i.e.:
#ifndef RCUTILS_SKIP_WARNING
# ifdef _MSC_VER
# pragma message(POINT_CLOUD_DEPRECATION_MESSAGE)
# else
# warning POINT_CLOUD_DEPRECATION_MESSAGE
# endif
#endifThere was a problem hiding this comment.
This is specifically to the bug in g++ that the preprocessor doesn't respect the pragmas. So it's better to have the pragma suppress the warning on Windows instead of compiling it out.
sensor_msgs/package.xml
Outdated
| @@ -15,6 +15,7 @@ | |||
| <build_depend>geometry_msgs</build_depend> | |||
| <build_depend>std_msgs</build_depend> | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
nit: seems like a leftover of a previous change
There was a problem hiding this comment.
yeah, i went through a lot of iterations. I'll squash this out.
| #ifndef __clang__ | ||
| #pragma GCC diagnostic ignored "-Wcpp" | ||
| #endif | ||
| #define RCUTILS_SKIP_WARNING |
There was a problem hiding this comment.
is this actually a macro defined in rcutils?
If not, I would use a name more specific to this package.
There was a problem hiding this comment.
Yeah, I was originally going to try to prototype it here and then move it upstream, but it go so complicated I'm going to need to defer the upstream proposal until after the freeze. I'll rename it for clarity for now. And we can replace it with a potential macro.
| @@ -18,7 +18,26 @@ | |||
| #include "sensor_msgs/msg/point_cloud.hpp" | |||
| #include "sensor_msgs/msg/point_cloud2.hpp" | |||
|
|
|||
| #ifdef _MSC_VER | |||
| #pragma warning(push) | |||
There was a problem hiding this comment.
this seem to be pushing the "warning configuration" state, but not adding any #pragma warning( disable : ...) in the middle
| #else | ||
| #pragma GCC diagnostic push | ||
| #ifndef __clang__ | ||
| #pragma GCC diagnostic ignored "-Wcpp" |
There was a problem hiding this comment.
I couldn't find the docs, is this for ignoring a #pragma message?
There was a problem hiding this comment.
If the answer is yes, you can ignore this other comment #107 (comment).
There was a problem hiding this comment.
Edit: sorry no, this isn't ignoring the windows message pragma. They aren't suppressed, but they also don't actually cause warnings so our compile tests don't complain.
Signed-off-by: Tully Foote <[email protected]>
tfoote
force-pushed
the
tfoote/deprecate_point_cloud
branch
from
d5148ab to
7893bce
Compare
|
Squashed with the feedback. Simplifying it for this specific use case and avoiding the more generic approaches that don't work in many cases.
|
|
|
||
| #ifdef _MSC_VER | ||
| #pragma warning(push) | ||
| #pragma warning(disable : 4996 ) |
There was a problem hiding this comment.
nit: seems to be an extra space before the closing bracket.
| @@ -68,7 +91,19 @@ TEST(sensor_msgs, PointCloudConversion) | |||
|
|
|||
| // Convert back to PointCloud | |||
| sensor_msgs::msg::PointCloud cloud3; | |||
| #ifdef _MSC_VER | |||
| #pragma warning(push) | |||
| #pragma warning(disable : 4996 ) | |||
Signed-off-by: Tully Foote <[email protected]>
Fixes #105