[Windows][master] remove GCC extension and alternative operator usage. #1583

seanyen · 2019-07-25T06:28:35Z

This change is to replace the gcc extension and alternative operator usage.

Replace alternative operator with primary tokens.
Replace variable length array with std::vector.
Don't do abi::__cxa_demangle when it is using MSVC toolchain.

bmagyar

I am not convinced that the array to std::vector changes were absolutely necessary but otherwise it looks good.

bmagyar · 2019-07-25T07:24:35Z

@seanyen please run clang-format on the code as per https://moveit.ros.org//documentation/contributing/code/

simonschmeisser · 2019-07-25T07:27:32Z

moveit_core/robot_state/src/robot_state.cpp

@@ -1231,7 +1231,7 @@ bool RobotState::getJacobian(const JointModelGroup* group, const LinkModel* link
    const JointModel* pjm = link->getParentJointModel();
    if (pjm->getVariableCount() > 0)
    {
-      if (not group->hasJointModel(pjm->getName()))
+      if (!group->hasJointModel(pjm->getName()))


what a shame MSVC is still not C++ standards compliant. wasn't there a compiler flag to enable a more compliant mode?

v4hn · 2019-07-29T12:07:07Z

.../planning_request_adapter/include/moveit/planning_request_adapter/planning_request_adapter.h

    std::string demangled_name = abi::__cxa_demangle(adapter_name.c_str(), NULL, NULL, &status);
+#endif


Not necessarily something that needs to be addressed in this pull-request, but I would vote for removing this ugly chunk altogether and making the initialize method pure virtual instead.

Any comments by other maintainers?

I agree that this is ugly and should probably be at least abstracted away, we could have a general demangle.h to get these names that'd know how to support different platform on it's own but yes, the best would be to remove hacky stuff like this altogether.

What if we drop the adapter_name variable and define a std::string getAdapterName() = 0;? This would enforce that whoever implement an adapter defines this function to return something. May as well return a correct name. Separate PR though, let's not hold this one up for that.

@bmagyar The logic above will always trigger when initialize is not overridden by the child class.
It's basically a run-time, warning-only version of a pure virtual initialize function.
There is no other place where the adapter name would be needed in the base class.

So instead of adding a new pure virtual method, as you propose, I proposed to enforce the warning condition at compile time by setting virtual void initialize(...) = 0.

davetcoleman · 2019-07-30T15:33:38Z

moveit_core/utils/src/robot_model_test_utils.cpp

@@ -120,14 +120,14 @@ void RobotModelBuilder::addChain(const std::string& section, const std::string&
    return;
  }
  // First link should already be added.
-  if (not urdf_model_->getLink(link_names[0]))
+  if (!urdf_model_->getLink(link_names[0]))


not this is not supported in windows?

not without setting the compiler to permissive mode or including a special header.
Personally, I prefer the primary tokens to be used throughout the code base, over including windows-specific headers.

.../planning_request_adapter/include/moveit/planning_request_adapter/planning_request_adapter.h

moveit_planners/chomp/chomp_motion_planner/src/chomp_trajectory.cpp

rhaschke · 2019-08-13T19:00:25Z

moveit_ros/planning/planning_scene_monitor/src/current_state_monitor.cpp

@@ -443,20 +443,20 @@ void planning_scene_monitor::CurrentStateMonitor::tfCallback()
        continue;
      joint_time_[joint] = latest_common_time;

-      double new_values[joint->getStateSpaceDimension()];
+      std::vector<double> new_values(joint->getStateSpaceDimension());


Ah. They don't support dynamic-size arrays.

rhaschke

Some minor fix required.

Co-Authored-By: Robert Haschke <rhaschke@users.noreply.github.com>

.../planning_request_adapter/include/moveit/planning_request_adapter/planning_request_adapter.h

Co-Authored-By: Robert Haschke <rhaschke@users.noreply.github.com>

replace the gcc extension and alternative operator usage Also guard use of Linux-specific abi header

replace the gcc extension and alternative operator usage

replace the gcc extension and alternative operator usage Also guard use of Linux-specific abi header

moveit#1583)

Remove GCC extension usage.

51d7c5e

seanyen requested review from bmagyar, mlautman and rhaschke as code owners July 25, 2019 06:28

bmagyar approved these changes Jul 25, 2019

View reviewed changes

simonschmeisser reviewed Jul 25, 2019

View reviewed changes

style fix

5a2dccf

seanyen changed the base branch from feature-windows-port to master July 25, 2019 22:36

seanyen changed the title ~~[Windows] remove GCC extension and alternative operator usage.~~ [Windows][master] remove GCC extension and alternative operator usage. Jul 25, 2019

v4hn reviewed Jul 29, 2019

View reviewed changes

davetcoleman reviewed Jul 30, 2019

View reviewed changes

rhaschke approved these changes Aug 13, 2019

View reviewed changes

rhaschke requested changes Aug 13, 2019

View reviewed changes

initialize status.

b5c0442

Co-Authored-By: Robert Haschke <rhaschke@users.noreply.github.com>

rhaschke reviewed Aug 13, 2019

View reviewed changes

.../planning_request_adapter/include/moveit/planning_request_adapter/planning_request_adapter.h Outdated Show resolved Hide resolved

Fix the indent.

ea2c3e0

Co-Authored-By: Robert Haschke <rhaschke@users.noreply.github.com>

rhaschke approved these changes Aug 13, 2019

View reviewed changes

v4hn merged commit f231ad7 into moveit:master Aug 14, 2019

v4hn mentioned this pull request Aug 14, 2019

pure virtual planning_request_adapter::initialize() #1621

Merged

seanyen added a commit to ms-iot/moveit that referenced this pull request Aug 22, 2019

remove GCC extension and alternative operator usage. (moveit#1583)

4aaf751

replace the gcc extension and alternative operator usage Also guard use of Linux-specific abi header

seanyen added a commit to seanyen/moveit that referenced this pull request Feb 8, 2020

remove GCC extension and alternative operator usage. (moveit#1583)

dc56bd5

replace the gcc extension and alternative operator usage

seanyen mentioned this pull request Feb 8, 2020

[melodic][cherry-pick] remove GCC extension and alternative operator usage. (#1583) #1884

Closed

6 tasks

rhaschke pushed a commit to ubi-agni/moveit that referenced this pull request Feb 8, 2020

remove GCC extension and alternative operator usage. (moveit#1583)

02443a3

replace the gcc extension and alternative operator usage Also guard use of Linux-specific abi header

sjahr pushed a commit to sjahr/moveit that referenced this pull request Jun 21, 2024

Do not add Pilz parameters to MoveIt Configs Utils if Pilz is not used (

1c7fa52

moveit#1583)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Windows][master] remove GCC extension and alternative operator usage. #1583

[Windows][master] remove GCC extension and alternative operator usage. #1583

seanyen commented Jul 25, 2019

bmagyar left a comment

bmagyar commented Jul 25, 2019

simonschmeisser Jul 25, 2019

v4hn Jul 29, 2019

bmagyar Jul 30, 2019

v4hn Jul 30, 2019 •

edited

Loading

davetcoleman Jul 30, 2019

v4hn Jul 30, 2019

rhaschke Aug 13, 2019

rhaschke left a comment

		std::string demangled_name = abi::__cxa_demangle(adapter_name.c_str(), NULL, NULL, &status);
		#endif

[Windows][master] remove GCC extension and alternative operator usage. #1583

[Windows][master] remove GCC extension and alternative operator usage. #1583

Conversation

seanyen commented Jul 25, 2019

bmagyar left a comment

Choose a reason for hiding this comment

bmagyar commented Jul 25, 2019

simonschmeisser Jul 25, 2019

Choose a reason for hiding this comment

v4hn Jul 29, 2019

Choose a reason for hiding this comment

bmagyar Jul 30, 2019

Choose a reason for hiding this comment

v4hn Jul 30, 2019 • edited Loading

Choose a reason for hiding this comment

davetcoleman Jul 30, 2019

Choose a reason for hiding this comment

v4hn Jul 30, 2019

Choose a reason for hiding this comment

rhaschke Aug 13, 2019

Choose a reason for hiding this comment

rhaschke left a comment

Choose a reason for hiding this comment

v4hn Jul 30, 2019 •

edited

Loading