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

clang-tidy fixes #264

Merged
merged 3 commits into from
Aug 29, 2020
Merged

clang-tidy fixes #264

merged 3 commits into from
Aug 29, 2020

Conversation

tylerjw
Copy link
Member

@tylerjw tylerjw commented Aug 26, 2020

No description provided.

@codecov
Copy link

codecov bot commented Aug 26, 2020

Codecov Report

Merging #264 into master will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #264      +/-   ##
==========================================
- Coverage   47.87%   47.87%   -0.01%     
==========================================
  Files         155      154       -1     
  Lines       15750    15749       -1     
==========================================
- Hits         7541     7540       -1     
  Misses       8209     8209              
Impacted Files Coverage Δ
...e/collision_detection_fcl/src/collision_common.cpp 76.99% <ø> (ø)
...processing/src/iterative_time_parameterization.cpp 92.35% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8a198ef...206b020. Read the comment docs.

@tylerjw
Copy link
Member Author

tylerjw commented Aug 26, 2020

Here is part of the diff from clang-tidy I didn't include. I'm unsure what is happening here.

diff --git a/moveit_core/collision_detection/include/moveit/collision_detection/collision_common.h b/moveit_core/collision_detection/include/moveit/collision_detection/collision_common.h
index 7e6b9df4b..ac97c9b49 100644
--- a/moveit_core/collision_detection/include/moveit/collision_detection/collision_common.h
+++ b/moveit_core/collision_detection/include/moveit/collision_detection/collision_common.h
@@ -300,9 +300,9 @@ struct DistanceRequest
 };
 
 /** \brief Generic representation of the distance information for a pair of objects */
-struct DistanceResultsData
+struct I0
 {
-  DistanceResultsData()
+  distanceResultsData()
   {
     clear();
   }
@@ -340,20 +340,20 @@ struct DistanceResultsData
   }
 
   /// Compare if the distance is less than another
-  bool operator<(const DistanceResultsData& other)
+  bool operator<(const DistanceRequest& other)
   {
     return (distance < other.distance);
   }
 
   /// Compare if the distance is greater than another
-  bool operator>(const DistanceResultsData& other)
+  bool operator>(const DistanceRequest& other)
   {
     return (distance > other.distance);
   }
 };
 
 /** \brief Mapping between the names of the collision objects and the DistanceResultData. */
-typedef std::map<const std::pair<std::string, std::string>, std::vector<DistanceResultsData> > DistanceMap;
+typedef std::map<const std::pair<std::string, std::string>, std::vector<DistanceRequest> > DistanceMap;
 
 /** \brief Result of a distance request. */
 struct DistanceResult
@@ -366,7 +366,7 @@ struct DistanceResult
   bool collision;
 
   /// ResultsData for the two objects with the minimum distance
-  DistanceResultsData minimum_distance;
+  DistanceResult minimum_distance;
 
   /// A map of distance data for each link in the req.active_components_only
   DistanceMap distances;
diff --git a/moveit_core/collision_detection_fcl/src/collision_common.cpp b/moveit_core/collision_detection_fcl/src/collision_common.cpp
index 33deadd0d..82259b02e 100644
--- a/moveit_core/collision_detection_fcl/src/collision_common.cpp
+++ b/moveit_core/collision_detection_fcl/src/collision_common.cpp
@@ -481,7 +481,7 @@ bool distanceCallback(fcl::CollisionObjectd* o1, fcl::CollisionObjectd* o2, void
     RCLCPP_DEBUG(LOGGER, "Actually checking collisions between %s and %s", cd1->getID().c_str(), cd2->getID().c_str());
 
   fcl::DistanceResultd fcl_result;
-  DistanceResultsData dist_result;
+  i0::DistanceResultsData dist_result;
   double dist_threshold = cdata->req->distance_threshold;
 
   const std::pair<std::string, std::string>& pc = cd1->getID() < cd2->getID() ?
@@ -653,7 +653,7 @@ struct IfSameType
 {
   enum
   {
-    value = 0
+    VALUE = 0
   };
 };
 
@@ -662,7 +662,7 @@ struct IfSameType<T, T>
 {
   enum
   {
-    value = 1
+    VALUE = 1
   };
 };

Copy link
Member

@henningkayser henningkayser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for preparing this. I'm also really confused about the other fixes above. Is this just some strange naming convention rule that goes wrong?

@@ -97,7 +97,7 @@ void collision_detection::CollisionEnvAllValid::checkRobotCollision(const Collis
}

void collision_detection::CollisionEnvAllValid::distanceRobot(const collision_detection::DistanceRequest& req,
collision_detection::DistanceResult& res,
DistanceResult& res,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this only applied to the result and not to the request...?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idk, I'm rather confused by clang-tidy. I created this diff following the instructions on moveit.ros.org for running it in a loop.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is some weird mismatch going on with the fcl namespace (which has all the same types as collision_detection/collision_common.h. The fcl package has a compat header that attempts to remap all the types, to me it looks like clang-tidy runs into issues resolving all those. I suppressed the weird "DistanceResultsData" renaming for now and added an issue for it (#267).

tylerjw and others added 3 commits August 29, 2020 10:26
* readability-identifier-naming
* performance-for-range-copy
* readability-named-parameter
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants