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

Visibility followup for marker #297

Merged
merged 2 commits into from
Dec 20, 2018
Merged

Visibility followup for marker #297

merged 2 commits into from
Dec 20, 2018

Conversation

Martin-Idel-SI
Copy link
Contributor

This PR exposes symbols of all classes. This is necessary when subclassing. Since warnings are being thrown on Windows, those are disabled by pragmas.

Here is my understanding on when this is safe:

  • It is always safe to call any method of this class, that's why method-wise symbol exporting works without warning.
  • When deriving from a class and just USING a type compiled inside the base class (e.g. in the form of a PIMPL or like the SelectionHandlers in markers), this is always safe.
  • Private members are safe (but may still throw warnings when they are from std - Microsoft itself says warnings can be ignored at that point, see https://msdn.microsoft.com/library/esew7y1w.aspx).
  • When deriving from a class and base class and derived class are compiled on the same system and with the same compiler.

It will only ever be problematic when a client derives from this class.

In our case, this implies it's mostly safe to set pragmas. The only problem I can see happening is when a client derives from e.g. MarkerBase and uses a different version of visualization_msgs::msg::Marker::ConstSharedPtr or rclcpp::Time. But I don't see how this can be done without breaking the ROS installation even earlier. To be sure, I added a comment explaining what's happening if ever somebody encounters a problem.

@wjwwood wjwwood added the in review Waiting for review (Kanban column) label Jun 13, 2018
@Martin-Idel-SI
Copy link
Contributor Author

Martin-Idel-SI commented Jun 13, 2018 •
edited
Loading

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

My bad. Since this is no crucial functionality, we'll look at it tomorrow.

@wjwwood wjwwood added in progress Actively being worked on (Kanban column) in review Waiting for review (Kanban column) and removed in review Waiting for review (Kanban column) in progress Actively being worked on (Kanban column) labels Jun 14, 2018
@Martin-Idel-SI Martin-Idel-SI mentioned this pull request Jun 15, 2018
Merged
@Martin-Idel-SI Martin-Idel-SI force-pushed the bugfix/visibility_followup_for_marker branch from 91f701a to fd34810 Compare June 15, 2018 15:23
@Martin-Idel-SI
Copy link
Contributor Author

CI after rebase:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

The current ros2 branch contains an MSBuild warning in map. This PR does not fix this issue.

@Martin-Idel-SI Martin-Idel-SI force-pushed the bugfix/visibility_followup_for_marker branch from fd34810 to d5ad808 Compare June 19, 2018 11:26
@Martin-Idel-SI
Copy link
Contributor Author

CI after rebase:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@Martin-Idel-SI Martin-Idel-SI force-pushed the bugfix/visibility_followup_for_marker branch from d5ad808 to 29937aa Compare June 20, 2018 10:58
@Martin-Idel-SI
Copy link
Contributor Author

CI after rebase:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@Martin-Idel-SI Martin-Idel-SI force-pushed the bugfix/visibility_followup_for_marker branch from 29937aa to e8eaab9 Compare June 21, 2018 13:22
@Martin-Idel-SI
Copy link
Contributor Author

CI after rebase

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@botteroa-si botteroa-si force-pushed the bugfix/visibility_followup_for_marker branch from e8eaab9 to 9d21bb4 Compare June 25, 2018 16:06
@botteroa-si
Copy link
Contributor

New CI after rebase:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@wjwwood wjwwood force-pushed the ros2 branch 2 times, most recently from 5997b63 to 5872a16 Compare June 28, 2018 03:30
@Martin-Idel-SI Martin-Idel-SI force-pushed the bugfix/visibility_followup_for_marker branch from 9d21bb4 to 7fc270e Compare June 28, 2018 10:22
@Martin-Idel-SI
Copy link
Contributor Author

What's going to happen with this PR? Shall I try to look a bit deeper or do we just wait for the release (I'm going to postpone new CIs for now).

@wjwwood wjwwood added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Dec 20, 2018
@wjwwood wjwwood merged commit a47ceb6 into ros2:ros2 Dec 20, 2018
@wjwwood wjwwood removed the in review Waiting for review (Kanban column) label Dec 20, 2018
@jacobperron jacobperron mentioned this pull request Dec 21, 2018
Closed
wjwwood added a commit that referenced this pull request Dec 21, 2018
@wjwwood
Revert "Visibility followup for marker (#297)"
ae4deb7 
This reverts commit a47ceb6.
@wjwwood wjwwood mentioned this pull request Dec 21, 2018
Merged
wjwwood added a commit that referenced this pull request Dec 21, 2018
@wjwwood
Revert "Visibility followup for marker" (#369)
712f903 
* Revert "Fix errors from uncrustify v0.68 (#366)"

This reverts commit 4d5e94d.

* Revert "Visibility followup for marker (#297)"

This reverts commit a47ceb6.
@clalancette clalancette mentioned this pull request Jan 8, 2019
Closed
@wjwwood wjwwood mentioned this pull request Jan 14, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wjwwood wjwwood wjwwood approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Martin-Idel-SI @botteroa-si @wjwwood