You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@kudu.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2017/06/09 02:23:06 UTC

[kudu-CR] Fix misc-move-const-arg tidy warnings

Hello Dan Burkert, Adar Dembo,

I'd like you to do a code review.  Please visit

    http://gerrit.cloudera.org:8080/7133

to review the following change.

Change subject: Fix misc-move-const-arg tidy warnings
......................................................................

Fix misc-move-const-arg tidy warnings

This was generated automatically using the '-fix' option from
clang-tidy.

Change-Id: I3b78afde183061112d69e422ca81b88c3fa2492d
---
M src/kudu/benchmarks/tpch/tpch_real_world.cc
M src/kudu/cfile/binary_dict_block.cc
M src/kudu/cfile/binary_plain_block.cc
M src/kudu/cfile/binary_prefix_block.cc
M src/kudu/client/meta_cache.cc
M src/kudu/common/generic_iterators.cc
M src/kudu/common/partition.cc
M src/kudu/common/partition_pruner-test.cc
M src/kudu/fs/file_block_manager.cc
M src/kudu/integration-tests/all_types-itest.cc
M src/kudu/integration-tests/alter_table-randomized-test.cc
M src/kudu/integration-tests/client-stress-test.cc
M src/kudu/integration-tests/dense_node-itest.cc
M src/kudu/integration-tests/external_mini_cluster-itest-base.cc
M src/kudu/integration-tests/external_mini_cluster-test.cc
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/flex_partitioning-itest.cc
M src/kudu/integration-tests/log-rolling-itest.cc
M src/kudu/integration-tests/master-stress-test.cc
M src/kudu/integration-tests/master_migration-itest.cc
M src/kudu/integration-tests/open-readonly-fs-itest.cc
M src/kudu/integration-tests/tablet_history_gc-itest.cc
M src/kudu/integration-tests/version_migration-test.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/rpc/acceptor_pool.cc
M src/kudu/tablet/diskrowset.cc
M src/kudu/tools/ksck-test.cc
M src/kudu/tools/ksck.cc
M src/kudu/tools/ksck_remote.cc
M src/kudu/tserver/heartbeater.cc
M src/kudu/util/failure_detector.cc
M src/kudu/util/metrics.cc
M src/kudu/util/pstack_watcher.cc
M src/kudu/util/resettable_heartbeater.cc
34 files changed, 46 insertions(+), 46 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/7133/1
-- 
To view, visit http://gerrit.cloudera.org:8080/7133
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I3b78afde183061112d69e422ca81b88c3fa2492d
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>

[kudu-CR] Fix misc-move-const-arg tidy warnings

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.

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 <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] tidy: fix misc-move-const-args

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.

Adar Dembo has posted comments on this change.

Change subject: tidy: fix misc-move-const-args
......................................................................


Patch Set 3: Code-Review+2

There are still some open comments from PS1 but they're not necessarily germane.

-- 
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: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No