Adar Dembo has posted comments on this change. Change subject: [iwyu] update std::move ......................................................................
Patch Set 1: (13 comments) Hmm, looks like our cpplint is making incorrect recommendations: Add #include <algorithm> for swap [build/include_what_you_use] [4] With C++11 std::swap moved to <utility>. Should we passing -std=c++11 or some such to cpplint? Or do we need a newer version? http://gerrit.cloudera.org:8080/#/c/8088/1/build-support/iwyu/mappings/kudu.imp File build-support/iwyu/mappings/kudu.imp: Line 34: { symbol: ["uint32", private, "\"kudu/gutil/integral_types.h\"", public] } This is interesting as we really shouldn't have any uint32 variable types in our code (they should be uint32_t). That said, there are uint32 references in a bunch of imported gutil code, so perhaps it'd be tough to cleanse them out. http://gerrit.cloudera.org:8080/#/c/8088/1/build-support/iwyu/mappings/libstdcpp.imp File build-support/iwyu/mappings/libstdcpp.imp: Line 31: { symbol: ["std::move", private, "<algorithm>", public ] }, Why this? I thought swap/move are now only in utility? http://gerrit.cloudera.org:8080/#/c/8088/1/src/kudu/consensus/raft_consensus_quorum-test.cc File src/kudu/consensus/raft_consensus_quorum-test.cc: Line 83 How does the removal of this (and the equivalent in consensus_meta-test) jive with this commit? Line 687: &last_replicate, &rounds)); Trailing whitespace. http://gerrit.cloudera.org:8080/#/c/8088/1/src/kudu/gutil/atomicops-internals-x86.cc File src/kudu/gutil/atomicops-internals-x86.cc: Line 33: // IWYU pragma: no_include "kudu/gutil/atomicops.h" Why is this necessary? Why shouldn't we include atomicops.h here? http://gerrit.cloudera.org:8080/#/c/8088/1/src/kudu/gutil/ref_counted.h File src/kudu/gutil/ref_counted.h: Line 10: #include <utility> // IWYU pragma: keep Why shouldn't we drop this include? http://gerrit.cloudera.org:8080/#/c/8088/1/src/kudu/tablet/tablet_history_gc-test.cc File src/kudu/tablet/tablet_history_gc-test.cc: Line 68 This change (from a macro to a function) makes sense, though again, I don't understand why it's part of this commit. http://gerrit.cloudera.org:8080/#/c/8088/1/src/kudu/tablet/transactions/transaction_tracker-test.cc File src/kudu/tablet/transactions/transaction_tracker-test.cc: Line 26: #include <google/protobuf/message.h> // IWYU pragma: keep Why do we need this include? Can we forward declare google::protobuf::Message instead? http://gerrit.cloudera.org:8080/#/c/8088/1/src/kudu/tools/tool_action_local_replica.cc File src/kudu/tools/tool_action_local_replica.cc: Line 32: #include <boost/container/flat_map.hpp> Hmm, what's this include for? http://gerrit.cloudera.org:8080/#/c/8088/1/src/kudu/tserver/tablet_copy_service-test.cc File src/kudu/tserver/tablet_copy_service-test.cc: Line 59 How is this related? http://gerrit.cloudera.org:8080/#/c/8088/1/src/kudu/twitter-demo/ingest_firehose.cc File src/kudu/twitter-demo/ingest_firehose.cc: Line 18: #include <fstream> // IWYU pragma: keep Why does IWYU want to drop this? We're using an std::ifstream in this file. http://gerrit.cloudera.org:8080/#/c/8088/1/src/kudu/util/file_cache-stress-test.cc File src/kudu/util/file_cache-stress-test.cc: Line 84: #define TEST_CHECK_OK(to_call) do { \ Why move this into a class definition? It's a macro so it isn't affected by namespaces... http://gerrit.cloudera.org:8080/#/c/8088/1/src/kudu/util/process_memory.cc File src/kudu/util/process_memory.cc: Line 23: #include <gperftools/malloc_extension.h> // IWYU pragma: keep Why does IWYU want to drop this? -- To view, visit http://gerrit.cloudera.org:8080/8088 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic5972f9a43f4c1db4a0dff5f983f7dba90c8db31 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
