Dan Burkert has posted comments on this change. Change subject: [rpc] use C++11 move semantics for selected methods ......................................................................
Patch Set 3: (3 comments) http://gerrit.cloudera.org:8080/#/c/6766/3/src/kudu/rpc/connection.cc File src/kudu/rpc/connection.cc: Line 154: const car_map_t::const_iterator end = awaiting_response_.end(); I would make these 'auto' typed, the iterator declarations are pretty noisy, and I think it's clear from context what they are. Line 159: for (auto it = awaiting_response_.begin(); it != end; ++it) { Move semantics are making the code significantly more complex here, and only to save a single copy during the loop. Given this is on a cold path, I don't think it's worth it. http://gerrit.cloudera.org:8080/#/c/6766/3/src/kudu/rpc/outbound_call.h File src/kudu/rpc/outbound_call.h: Line 162: void SetFailed(Status status, ErrorStatusPB* err_pb = nullptr); I'm a little bit conflicted about this patch. I really like this change to SetFailed, because it conveys the ownership semantics more clearly through types. I.e. SetFailed takes ownership of the status. That being said, in most of the places where this is called, it's done in a cold section of code (literally PREDICT_FALSE gated), and it's in the context of large functions where it's easy to lose track of whether something has been moved. What do you think about keeping this change, but not changing the cold-path call sites? -- To view, visit http://gerrit.cloudera.org:8080/6766 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id7ee8d69a620db597ed9d27c4284656a4169bb09 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
