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.

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

pure virtual planning_request_adapter::initialize() #1621

Merged
merged 1 commit into from
Aug 17, 2019

Conversation

v4hn
Copy link
Contributor

@v4hn v4hn commented Aug 14, 2019

The method was introduced in #1530, together with a TODO to make it pure virtual "in O-turtle".

A Windows-compatibility pr noticed that the current functionality is not available in Windows, as it uses cxxabi.h.
I would propose to either go for the pure virtual method directly or just implement an empty default implementation (apparently this was dismissed in #1530).

@v4hn v4hn requested a review from rhaschke as a code owner August 14, 2019 08:14
@rhaschke
Copy link
Contributor

We dismissed this approach in #1530, because this will leave existing plugins in a broken stage.
If we are willing to go this route now, I'm fine with it.

@v4hn
Copy link
Contributor Author

v4hn commented Aug 14, 2019

We dismissed this approach in #1530, because this will leave existing plugins in a broken stage.

Well, it is a breaking API change, yes. But I believe we agreed to allow for them in master.
The situation will not be much different for the master branch once we split of noetic-devel (i.e. for O-turtle).
Also, the current code breaks API just as well... only at runtime, but that's worse here in my opinion.

As I said, personally I'm open to have an empty default implementation too.

@v4hn v4hn added the awaits 2nd review one maintainer approved this request label Aug 16, 2019
@rhaschke rhaschke merged commit e7bc01e into moveit:master Aug 17, 2019
gleichdick pushed a commit to gleichdick/industrial_core that referenced this pull request Nov 13, 2019
PlanningRequestAdapter::initialize() is now pure virtual so we have
to provide an empty implementation on each subclass.

See also moveit/moveit#1621
gleichdick pushed a commit to gleichdick/industrial_core that referenced this pull request Nov 13, 2019
PlanningRequestAdapter::initialize() is now pure virtual so we have
to provide an empty implementation on each subclass.

See also moveit/moveit#1621
gleichdick pushed a commit to gleichdick/industrial_core that referenced this pull request Nov 15, 2019
PlanningRequestAdapter::initialize() is a pure virtual function on current
moveit master so we have to provide an empty implementation on each subclass.

See also moveit/moveit#1621
henningkayser pushed a commit to PickNikRobotics/moveit that referenced this pull request Nov 21, 2019
Implement initialize as pure virtual function.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaits 2nd review one maintainer approved this request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants