RSDK-3019 - add clang-tidy performance-move-const-arg #210
Conversation
stuqdog
requested review from
benjirewis and
njooma
and removed request for
a team
src/viam/sdk/rpc/dial.cpp
Outdated
|
|
||
| void DialOptions::set_timeout(std::chrono::duration<float> timeout) { | ||
| timeout_ = std::move(timeout); | ||
| timeout_ = timeout; |
There was a problem hiding this comment.
This one surprised me. It looks to me like the move was correct?
There was a problem hiding this comment.
(per error message) std::chrono::duration<float> is trivially copyable, so the move has no effect.
There was a problem hiding this comment.
OK I guess. I think it is overzealous. What if the type changed in the future and was no longer trivially copyable? Unlikely, I'll admit, for std::chrono::duration, but not for types we control. I don't see that the move has any downsides.
https://devblogs.microsoft.com/oldnewthing/20220512-00/?p=106651
Overall, I sort of feel like maybe we should disable that particular clang tidy check and just allow moves of trivial types. It is much easier to get SDK developers used to using move consistently if there isn't a gotcha about trivial types.
There was a problem hiding this comment.
Yeah, this makes sense. I've set performance-move-const-arg.CheckTriviallyCopyableMove to false and reinstated the move here, as well as the moves in geometry.cpp
benjirewis
left a comment
benjirewis
left a comment
There was a problem hiding this comment.
LGTM % Drew's comments on non-consting instead of moving.
src/viam/sdk/rpc/dial.cpp
Outdated
|
|
||
| void DialOptions::set_timeout(std::chrono::duration<float> timeout) { | ||
| timeout_ = std::move(timeout); | ||
| timeout_ = timeout; |
There was a problem hiding this comment.
(per error message) std::chrono::duration<float> is trivially copyable, so the move has no effect.
acmorrow
left a comment
acmorrow
left a comment
There was a problem hiding this comment.
This LGTM mod a few small things that don't, I think, warrant another round of review. I feel a bit like the warning on move of trivial is overzealous, but I don't feel strongly about it.
src/viam/sdk/rpc/dial.cpp
Outdated
|
|
||
| void DialOptions::set_timeout(std::chrono::duration<float> timeout) { | ||
| timeout_ = std::move(timeout); | ||
| timeout_ = timeout; |
There was a problem hiding this comment.
OK I guess. I think it is overzealous. What if the type changed in the future and was no longer trivially copyable? Unlikely, I'll admit, for std::chrono::duration, but not for types we control. I don't see that the move has any downsides.
https://devblogs.microsoft.com/oldnewthing/20220512-00/?p=106651
Overall, I sort of feel like maybe we should disable that particular clang tidy check and just allow moves of trivial types. It is much easier to get SDK developers used to using move consistently if there isn't a gotcha about trivial types.
src/viam/sdk/registry/registry.cpp
Outdated
| const std::lock_guard<std::mutex> 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, std::move(lock)); |
There was a problem hiding this comment.
Do you need the move on lock either? It's const.
There was a problem hiding this comment.
Ah, nope! Surprised it let us keep that. Fixed.
| this->short_names_ = new_short_names; | ||
|
|
||
| for (const auto& resource : resources) { | ||
| for (auto resource : resources) { |
There was a problem hiding this comment.
I disagree with this one, looking at it. This works because it now copies the pair out, and then moves from it. That doesn't seem worthwhile vs just viewing the pair and copying the second in the call to do_add
| } | ||
|
|
||
| void ResourceManager::do_add(const Name& name, const std::shared_ptr<Resource>& resource) { | ||
| void ResourceManager::do_add(Name name, std::shared_ptr<Resource> resource) { |
There was a problem hiding this comment.
This could be Name const& name, std::shared_ptr<Resource> resource) because the first parameter is only observed, but the second one is actually moved.
3 participants
performance-move-const-arginclang-tidy.clang-tidyto pass.