Adar Dembo has posted comments on this change. Change subject: Fix misc-move-const-arg tidy warnings ......................................................................
Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/7133/1/src/kudu/benchmarks/tpch/tpch_real_world.cc File src/kudu/benchmarks/tpch/tpch_real_world.cc: Line 177: cluster_.reset(new ExternalMiniCluster(opts)); Why did clang-tidy recommend this? I introduced these std::move() calls in commit 04e8ea2 and I remember Dan and I stared at the clang-tidy output wondering why it was recommending not to move. http://gerrit.cloudera.org:8080/#/c/7133/1/src/kudu/client/meta_cache.cc File src/kudu/client/meta_cache.cc: Line 817: InsertOrDie(&tablets_by_key, "", entry); I felt this pain recently too: InsertOrDie() (and variants) don't have an overload for providing rvalue references, so we're forced to copy the map value. http://gerrit.cloudera.org:8080/#/c/7133/1/src/kudu/common/generic_iterators.cc File src/kudu/common/generic_iterators.cc: PS1, Line 463: col_pred.second Maybe this should be 'pred'? http://gerrit.cloudera.org:8080/#/c/7133/1/src/kudu/common/partition.cc File src/kudu/common/partition.cc: Line 317: const string& upper = bound.second; If this were a non-const ref, could we move it? http://gerrit.cloudera.org:8080/#/c/7133/1/src/kudu/common/partition_pruner-test.cc File src/kudu/common/partition_pruner-test.cc: Line 784: ASSERT_OK(partition_schema.CreatePartitions(vector<KuduPartialRow>{ split }, I don't get this one; is it impossible to construct an initialize list with rvalue references? -- To view, visit http://gerrit.cloudera.org:8080/7133 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3b78afde183061112d69e422ca81b88c3fa2492d Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
