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

RoboticsYY · 2019-03-12T10:20:05Z

Description

Following up moveit#1366, this PR intends to enable the readability-identifier-naming clang-tidy fix to MoveIt code base. The commits are split for different variable cases and for easy web explorer loading. Please note:

For exceptions to method names: CartToJnt is preferred by KDL, which does not align with camelBack.
For exceptions to local variable names: Same name Qt component is declared in the class but not used.
For static const member variable names: the current .clang-tidy implements UPPER_CASE without a suffix _. If a suffix _ is preferred, readability-identifier-naming.ClassConstantSuffix is required in .clang-tidy.

Checklist

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

rhaschke

In general, I approve this PR. Changing case nicely exposed the wrong / unintended status for static / const of some variables. Could you, please, adapt these few variables?

To keep the current clean state also in future, I will enable the clang-tidy check for Travis.

rhaschke · 2019-03-12T12:16:14Z

moveit_planners/chomp/chomp_motion_planner/src/chomp_trajectory.cpp

@@ -208,12 +208,12 @@ void ChompTrajectory::fillInMinJerk()
 {
  double start_index = start_index_ - 1;
  double end_index = end_index_ + 1;
-  double T[6];  // powers of the time duration


I suggest to just rename T to t. Powers is a kind of misleading name here, but t[i] = t^i.

Agree, I think t or time is a better name here.

I agree powers is not a good name. But t is used in the following code block as //powers of the time index point, so I think maybe it's better to name T here as td and the following t as ti to distinguish them.

rhaschke · 2019-03-12T12:20:47Z

moveit_setup_assistant/src/widgets/end_effectors_widget.cpp

@@ -201,14 +201,14 @@ QWidget* EndEffectorsWidget::createEditWidget()
  controls_layout->addWidget(spacer);

  // Save
-  QPushButton* btn_save_ = new QPushButton("&Save", this);


Nice cleanup!

rhaschke · 2019-03-12T12:36:33Z

moveit_ros/planning/planning_scene_monitor/src/planning_scene_monitor.cpp

@@ -1398,16 +1398,16 @@ void PlanningSceneMonitor::configureDefaultPadding()
  }

  // Ensure no leading slash creates a bad param server address
-  static const std::string robot_description =


I don't see a reason for this variable to be static. This is local to this function, isn't it ?
Having it static introduces race conditions.
Other opinions?

Making it static saves the modified description so you don't have to substring it each call. But agreed; this function shouldn't be called that much, making it static is overkill.

Using a static variable here, also allows to use a single robot_description_ only, i.e. multiple instances of PSM cannot use different robot descriptions. I even consider this a bug, although it's very rare, that you have multiple robots in a single ROS environment..

I pushed a commit to fix this remaining issue.

rhaschke · 2019-03-12T12:44:28Z

moveit_core/trajectory_processing/src/time_optimal_trajectory_generation.cpp

@@ -59,29 +59,29 @@ class LinearPathSegment : public PathSegment
  {
  }

-  Eigen::VectorXd getConfig(double s) const
+  Eigen::VectorXd getConfig(double s) const override


I'm curious why these override issues were not caught by Travis.

I'm sure that clang-tidy related pkgs were cleaned up and reinstalled as it is in the Travis. But I'm not sure about the difference of the result, I will dig into that.

moveit_core/trajectory_processing/src/time_optimal_trajectory_generation.cpp

moveit_core/trajectory_processing/test/test_time_optimal_trajectory_generation.cpp

moveit_ros/planning/planning_request_adapter_plugins/src/add_time_optimal_parameterization.cpp

moveit_kinematics/test/test_kinematics_plugin.cpp

rhaschke · 2019-03-12T13:02:51Z

@RoboticsYY, I just noticed that this PR comprises a merge commit from master to include latest changes.
Please try to rebase instead. PR branches should avoid merge commits.

BryceStevenWilley

I also approve, with a few nits + @rhaschke's comments.

BryceStevenWilley · 2019-03-12T13:28:09Z

moveit_planners/chomp/chomp_motion_planner/src/chomp_trajectory.cpp

@@ -208,12 +208,12 @@ void ChompTrajectory::fillInMinJerk()
 {
  double start_index = start_index_ - 1;
  double end_index = end_index_ + 1;
-  double T[6];  // powers of the time duration


Agree, I think t or time is a better name here.

BryceStevenWilley · 2019-03-12T13:39:54Z

moveit_ros/planning/planning_scene_monitor/src/planning_scene_monitor.cpp

@@ -1398,16 +1398,16 @@ void PlanningSceneMonitor::configureDefaultPadding()
  }

  // Ensure no leading slash creates a bad param server address
-  static const std::string robot_description =


Making it static saves the modified description so you don't have to substring it each call. But agreed; this function shouldn't be called that much, making it static is overkill.

moveit_ros/planning_interface/move_group_interface/src/wrap_python_move_group.cpp

BryceStevenWilley · 2019-03-12T13:43:05Z

moveit_ros/planning_interface/robot_interface/src/wrap_python_robot_interface.cpp

@@ -373,33 +373,33 @@ static void wrap_robot_interface()
 {
  using namespace moveit;

-  bp::class_<RobotInterfacePython> RobotClass("RobotInterface", bp::init<std::string, bp::optional<std::string>>());


Same here: it's defining a class, should probably stay stylized as a class name.

RoboticsYY · 2019-03-13T07:53:17Z

Please try to rebase instead. PR branches should avoid merge commits.

Sorry for the mistake. The commit history has been cleaned up.

rhaschke

I approve this in the present form. @davetcoleman, could you provide another timely review?
This PR deals with variable renaming and you are the one who nitpicks on variable naming usually 😉

davetcoleman

No variable names got shorter, which is all i care about. The longer variable names are a great improvement thanks!

* Conform class name to `CamelCase` * Conform member method name to `camelBack` * Exceptions to method name * Conform local variable name to `lower_case` part 1 * Conform local variable name to `lower_case` part 2 * Conform local variable name to `lower_case` part 3 * Conform local variable name to `lower_case` part 4 * Local static variable to `lower_case` * Local variable manual fix * Exceptions to local variable name * Conform static const variable name to `UPPER_CASE` * Conform global variable name to `UPPER_CASE` * Conform static const member variable to `UPPER_CASE` * clang-format * Travis: mandatory clang-tidy-check * Catch up most recent changes * Update .clang-tidy * fixup! Conform static const variable name to `UPPER_CASE`

rhaschke reviewed Mar 12, 2019

View reviewed changes

rhaschke requested review from davetcoleman, v4hn and bmagyar and removed request for v4hn March 12, 2019 13:15

BryceStevenWilley approved these changes Mar 12, 2019

View reviewed changes

davetcoleman requested a review from henningkayser March 12, 2019 18:57

rhaschke mentioned this pull request Mar 12, 2019

Plugin based setup assistant #1375

Merged

RoboticsYY force-pushed the clang-tidy-fix branch 3 times, most recently from 0775589 to 3851661 Compare March 13, 2019 07:02

rhaschke approved these changes Mar 13, 2019

View reviewed changes

RoboticsYY force-pushed the clang-tidy-fix branch 2 times, most recently from 79d8fa4 to 6304f11 Compare March 13, 2019 09:20

RoboticsYY added 14 commits March 13, 2019 17:22

Conform class name to CamelCase

ec96b3f

Conform member method name to camelBack

59fe8ed

Exceptions to method name

0977d4e

Conform local variable name to lower_case part 1

d702230

Conform local variable name to lower_case part 2

6eecaf1

Conform local variable name to lower_case part 3

f78fc16

Conform local variable name to lower_case part 4

c1b5f83

Local static variable to lower_case

8cd8dfb

Local variable manual fix

0a42916

Exceptions to local variable name

5e0ea76

Conform static const variable name to UPPER_CASE

0bab3f4

Conform global variable name to UPPER_CASE

1a434e6

Conform static const member variable to UPPER_CASE

28c757e

Catch up most recent changes

ce1b301

RoboticsYY and others added 4 commits March 13, 2019 17:24

Update .clang-tidy

ac9d2ff

clang-format

9790d39

Travis: mandatory clang-tidy-check

0cf2221

fixup! Conform static const variable name to UPPER_CASE

bfe2a27

RoboticsYY force-pushed the clang-tidy-fix branch from 6304f11 to bfe2a27 Compare March 13, 2019 09:25

davetcoleman approved these changes Mar 14, 2019

View reviewed changes

davetcoleman merged commit 129e8e9 into moveit:master Mar 14, 2019

RoboticsYY deleted the clang-tidy-fix branch March 15, 2019 01:37

BryceStevenWilley mentioned this pull request Apr 24, 2019

Apply clang-tidy code linter #28

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

RoboticsYY commented Mar 12, 2019 •

edited

Loading

rhaschke left a comment

rhaschke Mar 12, 2019

BryceStevenWilley Mar 12, 2019

RoboticsYY Mar 13, 2019

rhaschke Mar 13, 2019

rhaschke Mar 12, 2019

rhaschke Mar 12, 2019

BryceStevenWilley Mar 12, 2019

rhaschke Mar 12, 2019

rhaschke Mar 13, 2019

rhaschke Mar 12, 2019

RoboticsYY Mar 13, 2019

rhaschke commented Mar 12, 2019

BryceStevenWilley left a comment

BryceStevenWilley Mar 12, 2019

BryceStevenWilley Mar 12, 2019

BryceStevenWilley Mar 12, 2019

RoboticsYY commented Mar 13, 2019

rhaschke left a comment

davetcoleman left a comment

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

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

Conversation

RoboticsYY commented Mar 12, 2019 • edited Loading

Description

Checklist

rhaschke 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhaschke commented Mar 12, 2019

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

RoboticsYY commented Mar 13, 2019

rhaschke left a comment

Choose a reason for hiding this comment

davetcoleman left a comment

Choose a reason for hiding this comment

RoboticsYY commented Mar 12, 2019 •

edited

Loading