diff --git a/.clang-tidy b/.clang-tidy index deda7293e..3f2373919 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -7,7 +7,6 @@ # readability-implicit-bool-conversion: We have decided that !ptr-type is cleaner than ptr-type==nullptr # readability-magic-numbers: This encourages useless variables and extra lint lines # performance-unnecessary-value-param: TODO(RSDK-3007) remove this and fix all lints. -# performance-move-const-arg: TODO(RSDK-3019) # misc-unused-parameters: TODO(RSDK-3008) remove this and fix all lints. # misc-include-cleaner: TODO(RSDK-5479) this is overly finnicky, add IWYU support and fix. # readability-function-cognitive-complexity: No, complexity is subjective and sometimes necessary. @@ -29,7 +28,6 @@ Checks: > -readability-implicit-bool-conversion, -readability-magic-numbers, -performance-unnecessary-value-param, - -performance-move-const-arg, -misc-unused-parameters, -misc-include-cleaner, -readability-function-cognitive-complexity, @@ -41,3 +39,5 @@ CheckOptions: value: true - key: readability-function-cognitive-complexity.Threshold value: 30 + - key: performance-move-const-arg.CheckTriviallyCopyableMove + value: false diff --git a/src/viam/sdk/registry/registry.cpp b/src/viam/sdk/registry/registry.cpp index 292401c53..67aa67fde 100644 --- a/src/viam/sdk/registry/registry.cpp +++ b/src/viam/sdk/registry/registry.cpp @@ -57,7 +57,7 @@ const Model& ModelRegistration::model() const { }; void Registry::register_model(std::shared_ptr resource) { - const std::string reg_key = resource->api().to_string() + "/" + resource->model().to_string(); + std::string reg_key = resource->api().to_string() + "/" + resource->model().to_string(); if (resources_.find(reg_key) != resources_.end()) { const std::string err = "Cannot add resource with name " + reg_key + "as it already exists"; throw std::runtime_error(err); @@ -95,14 +95,14 @@ std::shared_ptr Registry::lookup_model_inlock_( std::shared_ptr Registry::lookup_model(const std::string& name) { const std::lock_guard lock(lock_); - return lookup_model_inlock_(name, std::move(lock)); + return lookup_model_inlock_(name, lock); } std::shared_ptr Registry::lookup_model(const API& api, const Model& model) { const std::lock_guard lock(lock_); const std::string name = api.to_string() + "/" + model.to_string(); - return lookup_model_inlock_(std::move(name), std::move(lock)); + return lookup_model_inlock_(name, lock); } std::shared_ptr Registry::lookup_resource_server(const API& api) { diff --git a/src/viam/sdk/resource/resource_api.cpp b/src/viam/sdk/resource/resource_api.cpp index af3eb6d69..0f98c1d11 100644 --- a/src/viam/sdk/resource/resource_api.cpp +++ b/src/viam/sdk/resource/resource_api.cpp @@ -200,12 +200,12 @@ bool operator==(const Model& lhs, const Model& rhs) { RPCSubtype::RPCSubtype(API api, std::string proto_service_name, const google::protobuf::ServiceDescriptor& descriptor) - : descriptor_(std::move(descriptor)), + : descriptor_(descriptor), proto_service_name_(std::move(proto_service_name)), api_(std::move(api)) {} RPCSubtype::RPCSubtype(API api, const google::protobuf::ServiceDescriptor& descriptor) - : descriptor_(std::move(descriptor)), api_(std::move(api)) {} + : descriptor_(descriptor), api_(std::move(api)) {} const std::string& RPCSubtype::proto_service_name() const { return proto_service_name_; diff --git a/src/viam/sdk/resource/resource_manager.cpp b/src/viam/sdk/resource/resource_manager.cpp index 201b82856..795fa8c15 100644 --- a/src/viam/sdk/resource/resource_manager.cpp +++ b/src/viam/sdk/resource/resource_manager.cpp @@ -65,36 +65,35 @@ std::string get_shortcut_name(const std::string& name) { return name_split.at(name_split.size() - 1); } -void ResourceManager::do_add(const Name& name, const std::shared_ptr& resource) { +void ResourceManager::do_add(const Name& name, std::shared_ptr resource) { if (name.name().empty()) { throw "Empty name used for resource: " + name.to_string(); } - const std::string short_name = name.short_name(); + std::string short_name = name.short_name(); - do_add(short_name, resource); + do_add(std::move(short_name), std::move(resource)); } -void ResourceManager::do_add(const std::string& name, const std::shared_ptr& resource) { +void ResourceManager::do_add(std::string name, std::shared_ptr resource) { if (resources_.find(name) != resources_.end()) { throw "Attempted to add resource that already existed: " + name; } - resources_.emplace(name, resource); - - const std::string shortcut = get_shortcut_name(name); + std::string shortcut = get_shortcut_name(name); if (shortcut != name) { if (short_names_.find(shortcut) != short_names_.end()) { - short_names_.emplace(shortcut, ""); + short_names_.emplace(std::move(shortcut), ""); } else { - short_names_.emplace(shortcut, name); + short_names_.emplace(std::move(shortcut), name); } } + resources_.emplace(std::move(name), std::move(resource)); } -void ResourceManager::add(const Name& name, const std::shared_ptr& resource) { +void ResourceManager::add(const Name& name, std::shared_ptr resource) { const std::lock_guard lock(lock_); try { - do_add(name, resource); + do_add(name, std::move(resource)); } catch (std::exception& exc) { BOOST_LOG_TRIVIAL(error) << "Error adding resource to subtype service: " << exc.what(); } @@ -135,11 +134,11 @@ void ResourceManager::remove(const Name& name) { }; }; -void ResourceManager::replace_one(const Name& name, const std::shared_ptr& resource) { +void ResourceManager::replace_one(const Name& name, std::shared_ptr resource) { const std::lock_guard lock(lock_); try { do_remove(name); - do_add(name, resource); + do_add(name, std::move(resource)); } catch (std::exception& exc) { BOOST_LOG_TRIVIAL(error) << "failed to replace resource " << name.to_string() << ": " << exc.what(); @@ -151,10 +150,10 @@ const std::unordered_map>& ResourceManage return resources_; } -void ResourceManager::add(const std::string& name, const std::shared_ptr& resource) { +void ResourceManager::add(std::string name, std::shared_ptr resource) { const std::lock_guard lock(lock_); // NOLINTNEXTLINE(clang-analyzer-cplusplus.NewDeleteLeaks) - do_add(name, resource); + do_add(std::move(name), std::move(resource)); } } // namespace sdk diff --git a/src/viam/sdk/resource/resource_manager.hpp b/src/viam/sdk/resource/resource_manager.hpp index c48a2f1b6..0457162be 100644 --- a/src/viam/sdk/resource/resource_manager.hpp +++ b/src/viam/sdk/resource/resource_manager.hpp @@ -44,12 +44,12 @@ class ResourceManager { /// @brief Adds a single resource to the manager. /// @param name The name of the resource. /// @param resource The resource being added. - void add(const Name& name, const std::shared_ptr& resource); + void add(const Name& name, std::shared_ptr resource); /// @brief Adds a single resource to the manager. /// @param name The name of the resource. /// @param resource The resource being added. - void add(const std::string& name, const std::shared_ptr& resource); + void add(std::string name, std::shared_ptr resource); /// @brief Remodes a single resource from the manager. /// @param name The name of the resource to remove. @@ -58,7 +58,7 @@ class ResourceManager { /// @brief Replaces an existing resource. No-op if the named resource does not exist. /// @param name The name of the resource to replace. /// @param resource The new resource that is replacing the existing one. - void replace_one(const Name& name, const std::shared_ptr& resource); + void replace_one(const Name& name, std::shared_ptr resource); /// @brief Returns a reference to the existing resources within the manager. const std::unordered_map>& resources() const; @@ -70,8 +70,8 @@ class ResourceManager { std::unordered_map> resources_; /// @brief `short_names_` is a shortened version of `Name` N of form :. std::unordered_map short_names_; - void do_add(const Name& name, const std::shared_ptr& resource); - void do_add(const std::string& name, const std::shared_ptr& resource); + void do_add(const Name& name, std::shared_ptr resource); + void do_add(std::string name, std::shared_ptr resource); void do_remove(const Name& name); }; diff --git a/src/viam/sdk/rpc/server.cpp b/src/viam/sdk/rpc/server.cpp index 7988520c4..1b55ddef1 100644 --- a/src/viam/sdk/rpc/server.cpp +++ b/src/viam/sdk/rpc/server.cpp @@ -47,8 +47,9 @@ void Server::add_resource(std::shared_ptr resource) { << " but no matching resource server as found"; throw std::runtime_error(buffer.str()); } - auto resource_server = managed_servers_.at(std::move(api)); - resource_server->resource_manager()->add(resource->name(), std::move(resource)); + auto resource_server = managed_servers_.at(api); + auto name = resource->name(); + resource_server->resource_manager()->add(std::move(name), std::move(resource)); } void Server::start() { diff --git a/src/viam/sdk/services/mlmodel/client.cpp b/src/viam/sdk/services/mlmodel/client.cpp index d116161f4..923461e1a 100644 --- a/src/viam/sdk/services/mlmodel/client.cpp +++ b/src/viam/sdk/services/mlmodel/client.cpp @@ -69,6 +69,10 @@ std::shared_ptr MLModelServiceClient::infer( aav->views.emplace(kv.first, std::move(tensor)); } auto* const tsav_views = &aav->views; + // This move does nothing pre-C++20 because the `shared_ptr` aliasing constructor takes + // its first arg by `const&`. However, having it here is harmless, and in C++20 this + // move would be correct to minimize refcount modification. + // NOLINTNEXTLINE(performance-move-const-arg) return {std::move(aav), tsav_views}; } diff --git a/src/viam/sdk/services/mlmodel/server.cpp b/src/viam/sdk/services/mlmodel/server.cpp index 50dbfb37b..835699589 100644 --- a/src/viam/sdk/services/mlmodel/server.cpp +++ b/src/viam/sdk/services/mlmodel/server.cpp @@ -85,9 +85,8 @@ ::grpc::Status MLModelServiceServer::Metadata( *metadata_pb.mutable_type() = std::move(md.type); *metadata_pb.mutable_description() = std::move(md.description); - const auto pack_tensor_info = [&helper]( - auto& target, - const std::vector& source) { + const auto pack_tensor_info = [&helper](auto& target, + std::vector& source) { target.Reserve(source.size()); for (auto&& s : source) { auto& new_entry = *target.Add(); diff --git a/src/viam/sdk/services/motion/client.cpp b/src/viam/sdk/services/motion/client.cpp index 2f1d193be..cdb1fac76 100644 --- a/src/viam/sdk/services/motion/client.cpp +++ b/src/viam/sdk/services/motion/client.cpp @@ -92,7 +92,7 @@ pose_in_frame MotionClient::get_pose( .with(extra, [&](auto& request) { *request.mutable_component_name() = component_name.to_proto(); - *request.mutable_destination_frame() = std::move(destination_frame); + *request.mutable_destination_frame() = destination_frame; for (const auto& transform : supplemental_transforms) { *request.mutable_supplemental_transforms()->Add() = transform.to_proto(); } diff --git a/src/viam/sdk/services/motion/server.cpp b/src/viam/sdk/services/motion/server.cpp index 94edd590b..33bdb98a2 100644 --- a/src/viam/sdk/services/motion/server.cpp +++ b/src/viam/sdk/services/motion/server.cpp @@ -21,8 +21,6 @@ ::grpc::Status MotionServer::Move(::grpc::ServerContext* context, ::viam::service::motion::v1::MoveResponse* response) noexcept { return make_service_helper( "MotionServer::Move", this, request)([&](auto& helper, auto& motion) { - const pose_in_frame destination = pose_in_frame::from_proto(request->destination()); - const Name name = Name::from_proto(request->component_name()); std::shared_ptr ws; if (request->has_world_state()) { ws = std::make_shared(WorldState::from_proto(request->world_state())); @@ -34,8 +32,8 @@ ::grpc::Status MotionServer::Move(::grpc::ServerContext* context, Motion::constraints::from_proto(request->constraints())); } - const bool success = motion->move(std::move(destination), - std::move(name), + const bool success = motion->move(pose_in_frame::from_proto(request->destination()), + Name::from_proto(request->component_name()), std::move(ws), std::move(constraints), helper.getExtra()); @@ -49,13 +47,11 @@ ::grpc::Status MotionServer::MoveOnMap( ::viam::service::motion::v1::MoveOnMapResponse* response) noexcept { return make_service_helper( "MotionServer::MoveOnMap", this, request)([&](auto& helper, auto& motion) { - const auto& destination = pose::from_proto(request->destination()); - const auto& component_name = Name::from_proto(request->component_name()); - const auto& slam_name = Name::from_proto(request->slam_service_name()); - const bool success = motion->move_on_map(std::move(destination), - std::move(component_name), - std::move(slam_name), - helper.getExtra()); + const auto destination = pose::from_proto(request->destination()); + auto component_name = Name::from_proto(request->component_name()); + auto slam_name = Name::from_proto(request->slam_service_name()); + const bool success = motion->move_on_map( + destination, std::move(component_name), std::move(slam_name), helper.getExtra()); response->set_success(success); }); @@ -67,9 +63,9 @@ ::grpc::Status MotionServer::MoveOnGlobe( ::viam::service::motion::v1::MoveOnGlobeResponse* response) noexcept { return make_service_helper( "MotionServer::MoveOnGlobe", this, request)([&](auto& helper, auto& motion) { - const auto& destination = geo_point::from_proto(request->destination()); - const auto& component_name = Name::from_proto(request->component_name()); - const auto& movement_sensor_name = Name::from_proto(request->movement_sensor_name()); + const auto destination = geo_point::from_proto(request->destination()); + auto component_name = Name::from_proto(request->component_name()); + auto movement_sensor_name = Name::from_proto(request->movement_sensor_name()); std::vector obstacles; for (const auto& obstacle : request->obstacles()) { @@ -87,8 +83,8 @@ ::grpc::Status MotionServer::MoveOnGlobe( motion_configuration::from_proto(request->motion_configuration())); } - const std::string execution_id = motion->move_on_globe(std::move(destination), - std::move(heading), + const std::string execution_id = motion->move_on_globe(destination, + heading, std::move(component_name), std::move(movement_sensor_name), std::move(obstacles), @@ -103,37 +99,18 @@ ::grpc::Status MotionServer::GetPose( ::grpc::ServerContext* context, const ::viam::service::motion::v1::GetPoseRequest* request, ::viam::service::motion::v1::GetPoseResponse* response) noexcept { - if (!request) { - return ::grpc::Status(::grpc::StatusCode::INVALID_ARGUMENT, - "Called [GetPose] without a request"); - }; - - const std::shared_ptr rb = resource_manager()->resource(request->name()); - if (!rb) { - return grpc::Status(grpc::UNKNOWN, "resource not found: " + request->name()); - } - - const std::shared_ptr motion = std::dynamic_pointer_cast(rb); - - const auto& component_name = Name::from_proto(request->component_name()); - const std::string& destination_frame = request->destination_frame(); - std::vector supplemental_transforms; - for (const auto& proto_transform : request->supplemental_transforms()) { - supplemental_transforms.push_back(WorldState::transform::from_proto(proto_transform)); - } - AttributeMap extra; - if (request->has_extra()) { - extra = struct_to_map(request->extra()); - } - - const pose_in_frame pose = motion->get_pose(std::move(component_name), - std::move(destination_frame), - std::move(supplemental_transforms), - std::move(extra)); - - *response->mutable_pose() = pose.to_proto(); - - return ::grpc::Status(); + return make_service_helper( + "MotionServer::GetPose", this, request)([&](auto& helper, auto& motion) { + const auto& component_name = Name::from_proto(request->component_name()); + const std::string& destination_frame = request->destination_frame(); + std::vector supplemental_transforms; + for (const auto& proto_transform : request->supplemental_transforms()) { + supplemental_transforms.push_back(WorldState::transform::from_proto(proto_transform)); + } + const pose_in_frame pose = motion->get_pose( + component_name, destination_frame, supplemental_transforms, helper.getExtra()); + *response->mutable_pose() = pose.to_proto(); + }); }; ::grpc::Status MotionServer::GetPlan( diff --git a/src/viam/sdk/spatialmath/geometry.cpp b/src/viam/sdk/spatialmath/geometry.cpp index 20b593652..3325b6abd 100644 --- a/src/viam/sdk/spatialmath/geometry.cpp +++ b/src/viam/sdk/spatialmath/geometry.cpp @@ -130,7 +130,7 @@ std::vector GeometryConfig::from_proto( const viam::common::v1::GetGeometriesResponse& proto) { std::vector response; for (const auto& geometry : proto.geometries()) { - response.push_back(from_proto(std::move(geometry))); + response.push_back(from_proto(geometry)); } return response; }