Hello protobuf@, In the code below, do we really need to use std::move() when invoking candidate_logging_context->AddFeature() ? Please see the code snippet below.
The documentation for "Arenas, Messages and Pointers" contains examples that show the use of std::move() when invoking the Movers library functions. https://g3doc.corp.google.com/net/proto2/contrib/arena_utils/g3doc/arenas-ptrs-best-practices.md?cl=head However, candidate_logging_context.AddFeature() is not one of these Movers library functions, and I'm not sure it is a similar case. (Is it?) The source for AddFeature is here: https://cs.corp.google.com/piper///depot/google3/assistant/assistant_server/context/candidate_logging_context_impl.cc?l=355 There are some documents for std::move() that recommend against using std::move() when returning a value. I'm pretty sure that this advice applies here, since we are passing the value to an entirely different function. The value cannot be moved, so std::move() is a no-op. Am I misunderstanding something? https://g3doc.corp.google.com/devtools/library_club/g3doc/totw/labs/should-i-return-std-move.md?cl=head Thanks! My apologies if I have misunderstood. void AddTagOnToAssistantLogs( const assistant::btw::TagOn& tag_on, bool experimental, proto2::Arena* arena, CandidateLoggingContext* candidate_logging_context) { if (!tag_on.has_btw()) return; const auto* ve_type = GetFeatureByBtwResponseSource(tag_on.btw().source()); auto ve = MakeArenaSafeUnique<::logs::VisualElementProto>(arena); ve->mutable_assistant_btw_data()->set_rule_id(tag_on.btw().rule_id()); if (experimental) { // Not logging the BTW for counterfactual triggers. ve->set_visible( ::logs::VisualElementProto::VISIBILITY_REPRESSED_COUNTERFACTUAL); } else if (!tag_on.btw().is_personalized_content()) { ve->mutable_assistant_btw_data()->set_btw_text(tag_on.btw().text()); } if (tag_on.btw().secondary_ve_source() != BtwResponseSource::UNKNOWN) { auto secondary_ve = Copy(ve); const auto* secondary_ve_type = GetFeatureByBtwResponseSource(tag_on.btw().secondary_ve_source()); candidate_logging_context->AddFeature(secondary_ve_type, std::move(secondary_ve)); } candidate_logging_context->AddFeature(ve_type, std::move(ve)); } -- You received this message because you are subscribed to the Google Groups "Protocol Buffers" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/protobuf/ca28b4ca-5ec8-423d-841b-18dbf7481515%40googlegroups.com.
