clang-tidy: performance-*

v4hn · v4hn · commit 15707673ede0 · 2020-06-11T11:03:50.000+02:00
I NOLINTed the noexcept move constructor for Task for now because
the constructor *can* indeed throw exceptions.
diff --git a/.clang-tidy b/.clang-tidy
@@ -1,5 +1,6 @@
 ---
 Checks:          '-*,
+                  performance-*,
                   llvm-namespace-comment,
                   modernize-use-nullptr,
                   modernize-use-override,
diff --git a/core/include/moveit/task_constructor/container_p.h b/core/include/moveit/task_constructor/container_p.h
@@ -134,7 +134,8 @@ class ContainerBasePrivate : public StagePrivate
 	/// copy external_state to a child's interface and remember the link in internal_to map
 	void copyState(Interface::iterator external, const InterfacePtr& target, bool updated);
 	/// lift solution from internal to external level
-	void liftSolution(SolutionBasePtr solution, const InterfaceState* internal_from, const InterfaceState* internal_to);
+	void liftSolution(const SolutionBasePtr& solution, const InterfaceState* internal_from,
+	                  const InterfaceState* internal_to);
 
 	auto& internalToExternalMap() { return internal_to_external_; }
 	const auto& internalToExternalMap() const { return internal_to_external_; }
diff --git a/core/include/moveit/task_constructor/stage.h b/core/include/moveit/task_constructor/stage.h
@@ -370,7 +370,7 @@ class Connecting : public ComputeBase
 
 protected:
 	/// register solution as a solution connecting states from -> to
-	void connect(const InterfaceState& from, const InterfaceState& to, SolutionBasePtr solution);
+	void connect(const InterfaceState& from, const InterfaceState& to, const SolutionBasePtr& solution);
 
 	/// convienency methods consuming a SubTrajectory
 	void connect(const InterfaceState& from, const InterfaceState& to, SubTrajectory&& trajectory) {
diff --git a/core/include/moveit/task_constructor/stage_p.h b/core/include/moveit/task_constructor/stage_p.h
@@ -139,10 +139,10 @@ class StagePrivate
 	void composePropertyErrorMsg(const std::string& name, std::ostream& os);
 
 	// methods to spawn new solutions
-	void sendForward(const InterfaceState& from, InterfaceState&& to, SolutionBasePtr solution);
-	void sendBackward(InterfaceState&& from, const InterfaceState& to, SolutionBasePtr solution);
-	void spawn(InterfaceState&& state, SolutionBasePtr solution);
-	void connect(const InterfaceState& from, const InterfaceState& to, SolutionBasePtr solution);
+	void sendForward(const InterfaceState& from, InterfaceState&& to, const SolutionBasePtr& solution);
+	void sendBackward(InterfaceState&& from, const InterfaceState& to, const SolutionBasePtr& solution);
+	void spawn(InterfaceState&& state, const SolutionBasePtr& solution);
+	void connect(const InterfaceState& from, const InterfaceState& to, const SolutionBasePtr& solution);
 
 	bool storeSolution(const SolutionBasePtr& solution);
 	void newSolution(const SolutionBasePtr& solution);
diff --git a/core/include/moveit/task_constructor/stages/generate_pose.h b/core/include/moveit/task_constructor/stages/generate_pose.h
@@ -55,7 +55,7 @@ class GeneratePose : public MonitoringGenerator
 	bool canCompute() const override;
 	void compute() override;
 
-	void setPose(const geometry_msgs::PoseStamped pose) { setProperty("pose", std::move(pose)); }
+	void setPose(const geometry_msgs::PoseStamped& pose) { setProperty("pose", pose); }
 
 protected:
 	void onNewSolution(const SolutionBase& s) override;
diff --git a/core/include/moveit/task_constructor/task.h b/core/include/moveit/task_constructor/task.h
@@ -80,8 +80,8 @@ class Task : protected WrapperBase
 	              const std::string& adapter_plugins_param_name = "request_adapters");
 	Task(const std::string& id = "",
 	     ContainerBase::pointer&& container = std::make_unique<SerialContainer>("task pipeline"));
-	Task(Task&& other);
-	Task& operator=(Task&& other);
+	Task(Task&& other);  // NOLINT(performance-noexcept-move-constructor)
+	Task& operator=(Task&& other);  // NOLINT(performance-noexcept-move-constructor)
 	~Task() override;
 
 	const std::string& id() const;
diff --git a/core/src/container.cpp b/core/src/container.cpp
@@ -118,7 +118,7 @@ void ContainerBasePrivate::copyState(Interface::iterator external, const Interfa
 	internal_to_external_.insert(std::make_pair(&*internal, &*external));
 }
 
-void ContainerBasePrivate::liftSolution(SolutionBasePtr solution, const InterfaceState* internal_from,
+void ContainerBasePrivate::liftSolution(const SolutionBasePtr& solution, const InterfaceState* internal_from,
                                         const InterfaceState* internal_to) {
 	if (!storeSolution(solution))
 		return;
diff --git a/core/src/stage.cpp b/core/src/stage.cpp
@@ -140,7 +140,7 @@ bool StagePrivate::storeSolution(const SolutionBasePtr& solution) {
 	return true;
 }
 
-void StagePrivate::sendForward(const InterfaceState& from, InterfaceState&& to, SolutionBasePtr solution) {
+void StagePrivate::sendForward(const InterfaceState& from, InterfaceState&& to, const SolutionBasePtr& solution) {
 	assert(nextStarts());
 	if (!storeSolution(solution))
 		return;  // solution dropped
@@ -157,7 +157,7 @@ void StagePrivate::sendForward(const InterfaceState& from, InterfaceState&& to,
 	newSolution(solution);
 }
 
-void StagePrivate::sendBackward(InterfaceState&& from, const InterfaceState& to, SolutionBasePtr solution) {
+void StagePrivate::sendBackward(InterfaceState&& from, const InterfaceState& to, const SolutionBasePtr& solution) {
 	assert(prevEnds());
 	if (!storeSolution(solution))
 		return;  // solution dropped
@@ -174,7 +174,7 @@ void StagePrivate::sendBackward(InterfaceState&& from, const InterfaceState& to,
 	newSolution(solution);
 }
 
-void StagePrivate::spawn(InterfaceState&& state, SolutionBasePtr solution) {
+void StagePrivate::spawn(InterfaceState&& state, const SolutionBasePtr& solution) {
 	assert(prevEnds() && nextStarts());
 	if (!storeSolution(solution))
 		return;  // solution dropped
@@ -193,7 +193,7 @@ void StagePrivate::spawn(InterfaceState&& state, SolutionBasePtr solution) {
 	newSolution(solution);
 }
 
-void StagePrivate::connect(const InterfaceState& from, const InterfaceState& to, SolutionBasePtr solution) {
+void StagePrivate::connect(const InterfaceState& from, const InterfaceState& to, const SolutionBasePtr& solution) {
 	if (!storeSolution(solution))
 		return;  // solution dropped
 
@@ -742,7 +742,7 @@ bool Connecting::compatible(const InterfaceState& from_state, const InterfaceSta
 	return true;
 }
 
-void Connecting::connect(const InterfaceState& from, const InterfaceState& to, SolutionBasePtr s) {
+void Connecting::connect(const InterfaceState& from, const InterfaceState& to, const SolutionBasePtr& s) {
 	pimpl()->connect(from, to, s);
 }
 
diff --git a/core/src/stages/predicate_filter.cpp b/core/src/stages/predicate_filter.cpp
@@ -80,6 +80,8 @@ void PredicateFilter::init(const moveit::core::RobotModelConstPtr& robot_model)
 void PredicateFilter::onNewSolution(const SolutionBase& s) {
 	const auto& props = properties();
 
+	// false-positive in clang-tidy 10.0.0: predicate might change comment
+	// NOLINTNEXTLINE(performance-unnecessary-value-param)
 	std::string comment = s.comment();
 
 	double cost = s.cost();
diff --git a/core/src/task.cpp b/core/src/task.cpp
@@ -116,11 +116,12 @@ Task::Task(const std::string& id, ContainerBase::pointer&& container)
 		enableIntrospection(true);
 }
 
-Task::Task(Task&& other) : WrapperBase(new TaskPrivate(this, std::string()), std::make_unique<SerialContainer>()) {
+Task::Task(Task&& other)  // NOLINT(performance-noexcept-move-constructor)
+    : WrapperBase(new TaskPrivate(this, std::string()), std::make_unique<SerialContainer>()) {
 	*this = std::move(other);
 }
 
-Task& Task::operator=(Task&& other) {
+Task& Task::operator=(Task&& other) {  // NOLINT(performance-noexcept-move-constructor)
 	clear();  // remove all stages of current task
 	swap(this->pimpl_, other.pimpl_);
 	return *this;
@@ -133,7 +134,7 @@ struct PlannerCache
 	using ModelList = std::list<std::pair<std::weak_ptr<const robot_model::RobotModel>, PlannerMap> >;
 	ModelList cache_;
 
-	PlannerMap::mapped_type& retrieve(const robot_model::RobotModelConstPtr& model, PlannerID id) {
+	PlannerMap::mapped_type& retrieve(const robot_model::RobotModelConstPtr& model, const PlannerID& id) {
 		// find model in cache_ and remove expired entries while doing so
 		ModelList::iterator model_it = cache_.begin();
 		while (model_it != cache_.end()) {
diff --git a/visualization/visualization_tools/include/moveit/visualization_tools/marker_visualization.h b/visualization/visualization_tools/include/moveit/visualization_tools/marker_visualization.h
@@ -108,7 +108,7 @@ class MarkerVisualizationProperty : public rviz::BoolProperty
 	/// remove all hosted markers from display
 	void clearMarkers();
 	/// add markers in MarkerVisualization for display
-	void addMarkers(MarkerVisualizationPtr markers);
+	void addMarkers(const MarkerVisualizationPtr& markers);
 	/// update pose of all markers
 	void update(const planning_scene::PlanningScene& scene, const moveit::core::RobotState& robot_state);
 
diff --git a/visualization/visualization_tools/include/moveit/visualization_tools/task_solution_visualization.h b/visualization/visualization_tools/include/moveit/visualization_tools/task_solution_visualization.h
@@ -102,7 +102,7 @@ class TaskSolutionVisualization : public QObject
 	virtual void reset();
 
 	void onInitialize(Ogre::SceneNode* scene_node, rviz::DisplayContext* context);
-	void onRobotModelLoaded(moveit::core::RobotModelConstPtr robot_model);
+	void onRobotModelLoaded(const moveit::core::RobotModelConstPtr& robot_model);
 	void onEnable();
 	void onDisable();
 	void setName(const QString& name);
diff --git a/visualization/visualization_tools/src/marker_visualization.cpp b/visualization/visualization_tools/src/marker_visualization.cpp
@@ -188,7 +188,7 @@ void MarkerVisualizationProperty::clearMarkers() {
 	hosted_markers_.clear();
 }
 
-void MarkerVisualizationProperty::addMarkers(MarkerVisualizationPtr markers) {
+void MarkerVisualizationProperty::addMarkers(const MarkerVisualizationPtr& markers) {
 	if (!markers)
 		return;
 
diff --git a/visualization/visualization_tools/src/task_solution_visualization.cpp b/visualization/visualization_tools/src/task_solution_visualization.cpp
@@ -207,7 +207,7 @@ void TaskSolutionVisualization::setName(const QString& name) {
 		slider_dock_panel_->setWindowTitle(name + " - Slider");
 }
 
-void TaskSolutionVisualization::onRobotModelLoaded(robot_model::RobotModelConstPtr robot_model) {
+void TaskSolutionVisualization::onRobotModelLoaded(const robot_model::RobotModelConstPtr& robot_model) {
 	// Error check
 	if (!robot_model) {
 		ROS_ERROR_STREAM_NAMED("task_solution_visualization", "No robot model found");

Original file line number	Diff line number	Diff line change
`@@ -118,7 +118,7 @@ void ContainerBasePrivate::copyState(Interface::iterator external, const Interfa`
`118`	`118`	`internal_to_external_.insert(std::make_pair(&internal, &external));`
`119`	`119`	`}`
`120`	`120`
`121`		`-void ContainerBasePrivate::liftSolution(SolutionBasePtr solution, const InterfaceState* internal_from,`
	`121`	`+void ContainerBasePrivate::liftSolution(const SolutionBasePtr& solution, const InterfaceState* internal_from,`
`122`	`122`	`const InterfaceState* internal_to) {`
`123`	`123`	`if (!storeSolution(solution))`
`124`	`124`	`return;`
Original file line number	Diff line number	Diff line change
`@@ -140,7 +140,7 @@ bool StagePrivate::storeSolution(const SolutionBasePtr& solution) {`
`140`	`140`	`return true;`
`141`	`141`	`}`
`142`	`142`
`143`		`-void StagePrivate::sendForward(const InterfaceState& from, InterfaceState&& to, SolutionBasePtr solution) {`
	`143`	`+void StagePrivate::sendForward(const InterfaceState& from, InterfaceState&& to, const SolutionBasePtr& solution) {`
`144`	`144`	`assert(nextStarts());`
`145`	`145`	`if (!storeSolution(solution))`
`146`	`146`	`return; // solution dropped`
`@@ -157,7 +157,7 @@ void StagePrivate::sendForward(const InterfaceState& from, InterfaceState&& to,`
`157`	`157`	`newSolution(solution);`
`158`	`158`	`}`
`159`	`159`
`160`		`-void StagePrivate::sendBackward(InterfaceState&& from, const InterfaceState& to, SolutionBasePtr solution) {`
	`160`	`+void StagePrivate::sendBackward(InterfaceState&& from, const InterfaceState& to, const SolutionBasePtr& solution) {`
`161`	`161`	`assert(prevEnds());`
`162`	`162`	`if (!storeSolution(solution))`
`163`	`163`	`return; // solution dropped`
`@@ -174,7 +174,7 @@ void StagePrivate::sendBackward(InterfaceState&& from, const InterfaceState& to,`
`174`	`174`	`newSolution(solution);`
`175`	`175`	`}`
`176`	`176`
`177`		`-void StagePrivate::spawn(InterfaceState&& state, SolutionBasePtr solution) {`
	`177`	`+void StagePrivate::spawn(InterfaceState&& state, const SolutionBasePtr& solution) {`
`178`	`178`	`assert(prevEnds() && nextStarts());`
`179`	`179`	`if (!storeSolution(solution))`
`180`	`180`	`return; // solution dropped`
`@@ -193,7 +193,7 @@ void StagePrivate::spawn(InterfaceState&& state, SolutionBasePtr solution) {`
`193`	`193`	`newSolution(solution);`
`194`	`194`	`}`
`195`	`195`
`196`		`-void StagePrivate::connect(const InterfaceState& from, const InterfaceState& to, SolutionBasePtr solution) {`
	`196`	`+void StagePrivate::connect(const InterfaceState& from, const InterfaceState& to, const SolutionBasePtr& solution) {`
`197`	`197`	`if (!storeSolution(solution))`
`198`	`198`	`return; // solution dropped`
`199`	`199`
`@@ -742,7 +742,7 @@ bool Connecting::compatible(const InterfaceState& from_state, const InterfaceSta`
`742`	`742`	`return true;`
`743`	`743`	`}`
`744`	`744`
`745`		`-void Connecting::connect(const InterfaceState& from, const InterfaceState& to, SolutionBasePtr s) {`
	`745`	`+void Connecting::connect(const InterfaceState& from, const InterfaceState& to, const SolutionBasePtr& s) {`
`746`	`746`	`pimpl()->connect(from, to, s);`
`747`	`747`	`}`
`748`	`748`