Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Guard against DELETE being predefined on Windows #401

Merged
merged 3 commits into from
Aug 30, 2019
Merged

Guard against DELETE being predefined on Windows #401

merged 3 commits into from
Aug 30, 2019

Conversation

jacobperron
Copy link
Member

Motivated by build errors encountered building visualization_msgs/msg/Marker.msg.

@jacobperron
Guard against DELETE being predefined on Windows
828006d 
Motivated by build errors encountered building
visualization_msgs/msg/Marker.msg.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@jacobperron jacobperron mentioned this pull request Aug 23, 2019
Merged
@jacobperron
Copy link
Member Author

CI before and after: ros2/common_interfaces#70 (comment)

@jacobperron jacobperron added the in review Waiting for review (Kanban column) label Aug 23, 2019
@dirk-thomas
Copy link
Member

dirk-thomas commented Aug 23, 2019 •
edited
Loading

Instead of blindly undefining these names (even when the message doesn't use those names) I would prefer if this only happens in the case that the message does contain constants with that name. (Also see #118 and #272.)

@jacobperron
Update guard against common Windows preprocessor definitions
f5be052 
Rather than undefining unconditionally at the top of the file, only undefine
if a constant by one of the common names is defined.
Also, redefine the macro after the constant has been declared using the pragma
push_macro and pop_macro.
Suppress a potential warning for popping a macro that was not previously pushed.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@jacobperron
Copy link
Member Author

I've updated the PR so that we only undefine if a constant is using the same name.
Also, the original value of the macro is restored after defining the constant.

Ultimately, name mangling as described in ros2/design#172 is probably the way to go, but in order to unblock ros2/common_interfaces#70 we have this solution for now.

Windows CI:

@dirk-thomas
Copy link
Member

the original value of the macro is restored after defining the constant.

Is it still possible for code to use the constants? Is the current Jenkins build only testing if the generated code compiles or also if these constants can be used from other packages?

@jacobperron
Copy link
Member Author

Is it still possible for code to use the constants?

Good question. Though I built and tested ros-visualization/interactive_markers#44 as part of the job, it doesn't actually use the constant. I'll try triggering another job that includes code that actually uses one of the constants that are affected by this change.

@jacobperron
Copy link
Member Author

@dirk-thomas Looks like we can use the constants still. This build exercises tf2, which uses a constant with name NO_ERROR, and my rviz2 branch, which uses a constant with name DELETE: Build Status (warnings are unrelated)

@jacobperron
Alphabetize
9b902a8 
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@jacobperron jacobperron merged commit c488b81 into master Aug 30, 2019
@delete-merged-branch delete-merged-branch bot deleted the jacob/guard_predef_delete branch August 30, 2019 01:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dirk-thomas dirk-thomas dirk-thomas approved these changes

Assignees
No one assigned
Labels
in review Waiting for review (Kanban column)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jacobperron @dirk-thomas