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

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

Merged

Conversation

Projects
None yet
3 participants
@RoboticsYY
Copy link
Contributor

commented Feb 27, 2019

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
Copy link
Contributor

left a comment

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!

@@ -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;

This comment has been minimized.

Copy link
@BryceStevenWilley

BryceStevenWilley Feb 27, 2019

Contributor

Doesn't this change functionality?

This comment has been minimized.

Copy link
@RoboticsYY

RoboticsYY Feb 28, 2019

Author Contributor

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;
}

This comment has been minimized.

Copy link
@BryceStevenWilley

BryceStevenWilley Feb 28, 2019

Contributor

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.

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Feb 28, 2019

Member

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

This comment has been minimized.

Copy link
@RoboticsYY

RoboticsYY Feb 28, 2019

Author Contributor

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.

@@ -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;

This comment has been minimized.

Copy link
@BryceStevenWilley

BryceStevenWilley Feb 27, 2019

Contributor

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

This comment has been minimized.

Copy link
@RoboticsYY

RoboticsYY Feb 28, 2019

Author Contributor

Good to know a better solution for this.

@@ -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)

This comment has been minimized.

Copy link
@BryceStevenWilley

BryceStevenWilley Feb 27, 2019

Contributor

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.

This comment has been minimized.

Copy link
@RoboticsYY

RoboticsYY Feb 28, 2019

Author Contributor

Good idea to simplify in this way!

This comment has been minimized.

Copy link
@RoboticsYY

RoboticsYY Feb 28, 2019

Author Contributor

Code is further simplified in the suggested way.

@@ -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)

This comment has been minimized.

Copy link
@BryceStevenWilley

BryceStevenWilley Feb 27, 2019

Contributor

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

This comment has been minimized.

Copy link
@RoboticsYY

RoboticsYY Feb 28, 2019

Author Contributor

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 comment has been minimized.

Copy link
@BryceStevenWilley

BryceStevenWilley Feb 28, 2019

Contributor

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

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Feb 28, 2019

Member

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!

This comment has been minimized.

Copy link
@RoboticsYY

RoboticsYY Mar 4, 2019

Author Contributor

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

@@ -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)

This comment has been minimized.

Copy link
@BryceStevenWilley

BryceStevenWilley Feb 27, 2019

Contributor

Same with these here:

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

This comment has been minimized.

Copy link
@RoboticsYY

RoboticsYY Feb 28, 2019

Author Contributor

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

@@ -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)

This comment has been minimized.

Copy link
@BryceStevenWilley

BryceStevenWilley Feb 27, 2019

Contributor

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.

@@ -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)

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Feb 28, 2019

Member

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!

@@ -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;

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Feb 28, 2019

Member

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

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2019

Everything looks good to me. 👍

@davetcoleman davetcoleman merged commit 822a991 into ros-planning:melodic-devel Feb 28, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@welcome

This comment has been minimized.

Copy link

commented Feb 28, 2019

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

@RoboticsYY RoboticsYY deleted the RoboticsYY:clang-tidy-fix-melodic branch Mar 3, 2019

@rhaschke rhaschke referenced this pull request Mar 4, 2019

Merged

more clang-tidy-fix #1376

@RoboticsYY RoboticsYY referenced this pull request Mar 12, 2019

Merged

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

1 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.