RSDK-3007 - add clang-tidy performance-unnecessary-value-param #211
Conversation
|
|
||
| viam::common::v1::Vector3 Vector3::to_proto(const Vector3& vec) { | ||
| viam::common::v1::Vector3 Vector3::to_proto() const { | ||
| viam::common::v1::Vector3 result; | ||
| result.set_x(vec.x()); | ||
| result.set_y(vec.y()); | ||
| result.set_z(vec.z()); | ||
| result.set_x(x()); | ||
| result.set_y(y()); | ||
| result.set_z(z()); | ||
| return result; | ||
| }; |
There was a problem hiding this comment.
(flyby) this method was marked as static but just took an object ref as its only argument. I see no reason why this shouldn't just be a regular class method so I went ahead and made the change.
| viam::component::movementsensor::v1::GetPositionResponse MovementSensor::to_proto( | ||
| const position& position) { | ||
| component::movementsensor::v1::GetPositionResponse proto; | ||
| proto.set_altitude_m(position.altitude_m); | ||
| *proto.mutable_coordinate() = position.coordinate.to_proto(); | ||
| return proto; | ||
| } | ||
|
|
There was a problem hiding this comment.
(flyby) this was declared in the header but somehow never defined. That might mean it's better off to just delete it tbh, but it seemed more conservative to just add the implementation rq.
| #include <algorithm> | ||
| #include <chrono> | ||
| #include <cstddef> | ||
| #include <iostream> | ||
| #include <memory> | ||
| #include <mutex> | ||
| #include <ostream> | ||
| #include <set> | ||
| #include <string> | ||
| #include <thread> | ||
| #include <tuple> | ||
| #include <type_traits> |
stuqdog
requested review from
njooma and
purplenicole730
and removed request for
a team
acmorrow
left a comment
acmorrow
left a comment
There was a problem hiding this comment.
This LGTM. There are a few places where you opted to transform an existing value parameter into a const& but where the object looks like it is then copied from the &. In principle those could probably be moves. Basically:
If you start with:
void foo(Value v) {
do_something(v);
}
You could either go to:
void foo(const Value& v) { // No copy here
do_something(v); // Copy here
}
Or
void foo(Value v) { // Copy OR move here; depends on caller
do_something(std::move(v)); // Move here
}
So the former solution is a guaranteed copy in the implementation. The latter solution is either a copy and a move or two moves, depending on whether the caller provides an lvalue or an rvalue.
Anyway, I'm not proposing you go through this review and adjust. I think what you have here is fine especially if you are doing it primarily to placate the linter.
src/viam/sdk/module/handler_map.cpp
Outdated
| } | ||
|
|
||
| void HandlerMap_::add_model(Model model, RPCSubtype subtype) { | ||
| void HandlerMap_::add_model(const Model& model, const RPCSubtype& subtype) { |
There was a problem hiding this comment.
I wonder if it would still want this as a & if you std::move'd model below.
src/viam/sdk/robot/client.cpp
Outdated
| }; | ||
|
|
||
| RobotClient::RobotClient(std::shared_ptr<ViamChannel> channel) | ||
| RobotClient::RobotClient(const std::shared_ptr<ViamChannel>& channel) |
There was a problem hiding this comment.
Could also leave it a value and std::move it on 220 probably.
There was a problem hiding this comment.
Ahh good catch, thanks! Fixed.
src/viam/sdk/robot/client.cpp
Outdated
|
|
||
| std::shared_ptr<RobotClient> RobotClient::with_channel(std::shared_ptr<ViamChannel> channel, | ||
| Options options) { | ||
| std::shared_ptr<RobotClient> RobotClient::with_channel(const std::shared_ptr<ViamChannel>& channel, |
There was a problem hiding this comment.
Ditto re maybe a std::move below makes if OK to pass by value?
…o performance-unnecessary-value-param
2 participants
to_protomethodVector3::to_prototo no longer be static