Apply clang tidy fix to entire code base (Part 1) #1366

RoboticsYY · 2019-02-27T02:05:49Z

Description

Redo moveit#1354 for Melodic.

Since the clang-tidy fixes can involve many file changes, which makes it difficult for reviewing. The previous PR will be split into several PRs. This is the first one which contains some low risk fixes. Please note clang-tidy-3.9 is used, and there are some manual fixes to the auto clang-tidy fix, they are related to:

performance-unnecessary-copy-initialization may falsely convert non-const variable to const variable
modernize-use-nullptr may convert some unnecessary NULL and 0 to nullptr.
- Boost function pointer initialization by nullptr seems not allowed.
- Some NULL and 0 refer to int or enum or null Qstring, it may not be safe to replace them by nullptr.
readability-simplify-boolean-expr may convert if(...){...}...return... clause by return... clause. But some code cannot be ignored.

Checklist

Required by CI: Code is auto formatted using [clang-format]

BryceStevenWilley

I think that manual fixes for readability-simplify-boolean-expr aren't necessary, but that's my only sticking point for this PR. Looks good!

BryceStevenWilley · 2019-02-27T17:41:15Z

moveit_setup_assistant/src/widgets/default_collisions_widget.cpp

@@ -420,14 +420,14 @@ void DefaultCollisionsWidget::showHeaderContextMenu(const QPoint& p)
    menu.addActions(header_actions_);
  menu.exec(global);

-  clicked_headers_ = 0;
-  clicked_section_ = -1;
+  // clicked_headers_ = nullptr;


Doesn't this change functionality?

From the whole function pasted below, seems these two operations override the operations in the previous if...else if...else... clause, which I think is not necessary.

void DefaultCollisionsWidget::showHeaderContextMenu(const QPoint& p) { // This method might be triggered from either of the headers QPoint global; if (sender() == collision_table_->verticalHeader()) { clicked_section_ = collision_table_->verticalHeader()->logicalIndexAt(p); clicked_headers_ = Qt::Vertical; global = collision_table_->verticalHeader()->mapToGlobal(p); } else if (sender() == collision_table_->horizontalHeader()) { clicked_section_ = collision_table_->horizontalHeader()->logicalIndexAt(p); clicked_headers_ = Qt::Horizontal; global = collision_table_->horizontalHeader()->mapToGlobal(p); } else { clicked_section_ = -1; clicked_headers_ = Qt::Horizontal | Qt::Vertical; } QMenu menu; if (clicked_section_ < 0) menu.addAction(header_actions_.at(0)); // only 'show' action else menu.addActions(header_actions_); menu.exec(global); // clicked_headers_ = nullptr; // clicked_section_ = -1; }

True, but there are other functions in this class that don't set these values, meaning this function likely needs to end by setting them to their default values.

to keep this PR from getting held up, i think we should err on keeping things the way they are in issues like this

Maybe it's better to keep these two lines for now. I think this is beyond a clang-tidy topic. clicked_headers_ = {} is a better option.

BryceStevenWilley · 2019-02-27T17:44:04Z

moveit_setup_assistant/src/widgets/default_collisions_widget.cpp

@@ -420,14 +420,14 @@ void DefaultCollisionsWidget::showHeaderContextMenu(const QPoint& p)
    menu.addActions(header_actions_);
  menu.exec(global);

-  clicked_headers_ = 0;
-  clicked_section_ = -1;
+  // clicked_headers_ = nullptr;


According to a Qt mailing list thread, you can use clicked_headers_ = {}.

Good to know a better solution for this.

BryceStevenWilley · 2019-02-27T17:51:09Z

moveit_planners/ompl/ompl_interface/src/detail/state_validity_checker.cpp

@@ -214,7 +214,7 @@ bool ompl_interface::StateValidityChecker::isValidWithCache(const ompl::base::St
  collision_detection::CollisionResult res;
  planning_context_->getPlanningScene()->checkCollision(
      verbose ? collision_request_simple_verbose_ : collision_request_simple_, res, *robot_state);
-  if (!res.collision)
+  if (!res.collision)  // NOLINT(readability-simplify-boolean-expr)


I think this can still be simplified though:

if (!res.collision) { const_cast<ob::State*>(state)->as<ModelBasedStateSpace::StateType>()->markValid(); } else { const_cast<ob::State*>(state)->as<ModelBasedStateSpace::StateType>()->markInvalid(); } return !res.collision;

There shouldn't be any need to mark them NOLINT.

Good idea to simplify in this way!

Code is further simplified in the suggested way.

BryceStevenWilley · 2019-02-27T17:54:07Z

moveit_kinematics/srv_kinematics_plugin/src/srv_kinematics_plugin.cpp

@@ -171,7 +171,7 @@ bool SrvKinematicsPlugin::getPositionIK(const geometry_msgs::Pose& ik_pose, cons
                                        std::vector<double>& solution, moveit_msgs::MoveItErrorCodes& error_code,
                                        const kinematics::KinematicsQueryOptions& options) const
 {
-  const IKCallbackFn solution_callback = 0;
+  const IKCallbackFn solution_callback = 0;  // NOLINT(modernize-use-nullptr)


BTW, great idea adding the check after the NOLINT. Does clang recognize that, or is that just for documentation?

Yes, clang recognize this. Please see the guidelines. Maybe this should also be added to the style guidelines on MoveIt! website similar to clang-format.

This definitely should be added to the docs, I missed that fact when first doing clang-tidy.

Yes please add a new section to https://github.com/ros-planning/moveit.ros.org/blob/gh-pages/documentation/contributing/code/index.markdown documenting how to ignore some clang-tidy rules!

@davetcoleman @BryceStevenWilley The exception rules of clang-tidy were added to the style guidelines in moveit.ros.org#258. Please help review.

BryceStevenWilley · 2019-02-27T17:59:13Z

moveit_planners/ompl/ompl_interface/src/model_based_planning_context.cpp

@@ -406,7 +406,7 @@ void ompl_interface::ModelBasedPlanningContext::convertPath(const ompl::geometri
 bool ompl_interface::ModelBasedPlanningContext::getSolutionPath(robot_trajectory::RobotTrajectory& traj) const
 {
  traj.clear();
-  if (!ompl_simple_setup_->haveSolutionPath())
+  if (!ompl_simple_setup_->haveSolutionPath())  // NOLINT(readability-simplify-boolean-expr)


Same with these here:

if (ompl_simple_setup_->haveSolutionPath()) { convertPath(ompl_simple_setup_->getSolutionPath(), traj); } return ompl_simple_setup_->haveSolutionPath();

This one I would rather to keep NOLINT. Convert in this way could make it more expensive.

BryceStevenWilley · 2019-02-27T18:04:06Z

moveit_planners/ompl/ompl_interface/src/constraints_library.cpp

@@ -461,7 +461,7 @@ ompl::base::StateStoragePtr ompl_interface::ConstraintsLibrary::constructConstra

  // construct the constrained states

-  robot_state::RobotState robot_state(default_state);
+  robot_state::RobotState robot_state(default_state);  // NOLINT(performance-unnecessary-copy-initialization)


Does this not compile after linting, or does it just add a bug implicitly? You're correct; this shouldn't be const. I just didn't think that clang-tidy would put the project in a state where it can't compile.

davetcoleman · 2019-02-28T05:43:43Z

moveit_kinematics/srv_kinematics_plugin/src/srv_kinematics_plugin.cpp

@@ -171,7 +171,7 @@ bool SrvKinematicsPlugin::getPositionIK(const geometry_msgs::Pose& ik_pose, cons
                                        std::vector<double>& solution, moveit_msgs::MoveItErrorCodes& error_code,
                                        const kinematics::KinematicsQueryOptions& options) const
 {
-  const IKCallbackFn solution_callback = 0;
+  const IKCallbackFn solution_callback = 0;  // NOLINT(modernize-use-nullptr)


Yes please add a new section to https://github.com/ros-planning/moveit.ros.org/blob/gh-pages/documentation/contributing/code/index.markdown documenting how to ignore some clang-tidy rules!

davetcoleman · 2019-02-28T05:57:07Z

moveit_setup_assistant/src/widgets/default_collisions_widget.cpp

@@ -420,14 +420,14 @@ void DefaultCollisionsWidget::showHeaderContextMenu(const QPoint& p)
    menu.addActions(header_actions_);
  menu.exec(global);

-  clicked_headers_ = 0;
-  clicked_section_ = -1;
+  // clicked_headers_ = nullptr;


to keep this PR from getting held up, i think we should err on keeping things the way they are in issues like this

BryceStevenWilley · 2019-02-28T17:48:36Z

Everything looks good to me. 👍

welcome · 2019-02-28T19:22:30Z

Congrats on getting your first MoveIt! pull request merged and improving open source robotics!

RoboticsYY added 15 commits February 25, 2019 16:41

clang-tidy: performance-faster-string-find

ab7a036

clang-tidy: performance-for-range-copy

381359e

clang-tidy: performance-unnecessary-copy-initialization

57e14c8

clang-tidy: clang-tidy: performance-unnecessary-value-param

c9abd65

clang-tidy: modernize-redundant-void-arg

5850ee4

clang-tidy: modernize-use-nullptr

feb9abe

clang-tidy: modernize-use-default

f2bb244

clang-tidy: readability-named-parameter

541c6db

clang-tidy: readability-redundant-string-cstr

e437bc7

clang-tidy: readability-simplify-boolean-expr

ddd0c95

clang-tidy: readability-simplify-boolean-expr (manual fix)

c518ad6

clang-tidy: readability-container-size-empty

830b0f9

clang-tidy: llvm-namespace-comment

b73a193

Update .clang-tidy with run checks

f6b6ded

clang-format

de1f30c

BryceStevenWilley suggested changes Feb 27, 2019

View reviewed changes

davetcoleman approved these changes Feb 28, 2019

View reviewed changes

RoboticsYY added 2 commits February 28, 2019 15:46

Keep default values setting for QFlags variable

2286407

Further simplify boolean expression

e100780

davetcoleman approved these changes Feb 28, 2019

View reviewed changes

davetcoleman merged commit 822a991 into moveit:melodic-devel Feb 28, 2019

RoboticsYY deleted the clang-tidy-fix-melodic branch March 3, 2019 08:50

RoboticsYY mentioned this pull request Mar 4, 2019

Update clang-tidy documentation with exceptions note moveit/moveit.ros.org#258

Merged

rhaschke mentioned this pull request Mar 4, 2019

more clang-tidy-fix #1376

Merged

RoboticsYY mentioned this pull request Mar 12, 2019

Apply clang tidy fix to entire code base (Part 2) #1394

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply clang tidy fix to entire code base (Part 1) #1366

Apply clang tidy fix to entire code base (Part 1) #1366

RoboticsYY commented Feb 27, 2019 •

edited

BryceStevenWilley left a comment

BryceStevenWilley Feb 27, 2019

RoboticsYY Feb 28, 2019

BryceStevenWilley Feb 28, 2019

davetcoleman Feb 28, 2019

RoboticsYY Feb 28, 2019

BryceStevenWilley Feb 27, 2019

RoboticsYY Feb 28, 2019

BryceStevenWilley Feb 27, 2019

RoboticsYY Feb 28, 2019

RoboticsYY Feb 28, 2019 •

edited

BryceStevenWilley Feb 27, 2019

RoboticsYY Feb 28, 2019

BryceStevenWilley Feb 28, 2019

davetcoleman Feb 28, 2019

RoboticsYY Mar 4, 2019

BryceStevenWilley Feb 27, 2019

RoboticsYY Feb 28, 2019 •

edited

BryceStevenWilley Feb 27, 2019

davetcoleman Feb 28, 2019

davetcoleman Feb 28, 2019

BryceStevenWilley commented Feb 28, 2019

welcome bot commented Feb 28, 2019

Apply clang tidy fix to entire code base (Part 1) #1366

Apply clang tidy fix to entire code base (Part 1) #1366

Conversation

RoboticsYY commented Feb 27, 2019 • edited

Description

Checklist

BryceStevenWilley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RoboticsYY Feb 28, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RoboticsYY Feb 28, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BryceStevenWilley commented Feb 28, 2019

welcome bot commented Feb 28, 2019

RoboticsYY commented Feb 27, 2019 •

edited

RoboticsYY Feb 28, 2019 •

edited

RoboticsYY Feb 28, 2019 •

edited