Alexey Serbin has posted comments on this change. Change subject: [iwyu] update std::move ......................................................................
Patch Set 1: (26 comments) Thank you for the review. 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 i Good observation -- I'll replace [u]int32 with [u]int32_t in our code (keeping the gutil's code unmodified as much as possible) and send it as a separate changelist then. 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? Because of http://en.cppreference.com/w/cpp/algorithm/move http://gerrit.cloudera.org:8080/#/c/8088/1/src/kudu/benchmarks/tpch/tpch_real_world.cc File src/kudu/benchmarks/tpch/tpch_real_world.cc: Line 40: // TODO Make the inserts multi-threaded. See Kudu-629 for the technique. > warning: missing username/bug in TODO [google-readability-todo] Done 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) ji It's two-fold: 1) Because of a bug/feature in IWYU the tool wants to know about the class that hosts the method ReplicateSequenceOfMessages() prior to this macro, so it recommends to add forward declaration of various test classes prior to this macro. So, this change serves as a work-around for that bug/feature. 2) Because I didn't see much of value in introducing this local macro. Line 687: &last_replicate, &rounds)); > Trailing whitespace. Done 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? This is necessary to overcome some IWYU bug. Otherwise, IWYU would suggest to add that header into this file, which would break compilation. 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? Because due to some IWYU bug the tool would suggest to remove this header file even with the new mappings. http://gerrit.cloudera.org:8080/#/c/8088/1/src/kudu/gutil/strings/strip.cc File src/kudu/gutil/strings/strip.cc: Line 20: using std::swap; > warning: using decl 'swap' is unused [misc-unused-using-decls] Done http://gerrit.cloudera.org:8080/#/c/8088/1/src/kudu/gutil/strings/util.cc File src/kudu/gutil/strings/util.cc: Line 29: using std::max; > warning: using decl 'max' is unused [misc-unused-using-decls] Done http://gerrit.cloudera.org:8080/#/c/8088/1/src/kudu/integration-tests/master-stress-test.cc File src/kudu/integration-tests/master-stress-test.cc: Line 27: #include <glog/logging.h> > warning: #includes are not sorted properly [llvm-include-order] Done http://gerrit.cloudera.org:8080/#/c/8088/1/src/kudu/integration-tests/version_migration-test.cc File src/kudu/integration-tests/version_migration-test.cc: Line 19: #include <ostream> > warning: #includes are not sorted properly [llvm-include-order] Done http://gerrit.cloudera.org:8080/#/c/8088/1/src/kudu/tablet/compaction-test.cc File src/kudu/tablet/compaction-test.cc: Line 26: #include <vector> > warning: #includes are not sorted properly [llvm-include-order] Done 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 Ah, all right -- I'll publish it separately. For the explanation why I did this please check the similar change in one of the files in the changelist closer to the top of the list. Line 110: int64_t TotalNumRows() const { return num_rowsets_ * rows_per_rowset_; } > warning: either cast from 'int' to 'int64_t' (aka 'long') is ineffective, o Done 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::Messa That's the strangest thing I hit so far with IWYU. Removing this header and adding the forward declaration for google::protobuf::Message will result in src/kudu/tablet/transactions/transaction_tracker-test.cc should add these lines: #include <google/protobuf/message.h> // for Message src/kudu/tablet/transactions/transaction_tracker-test.cc should remove these lines: namespace google { namespace protobuf { class Message; } } Adding 'IWYU pragma: keep' for the forward declaration does not work in this case (probably, just another bug). So, I decided to go with including that header and adding the pragma which at least works. The alternative is to keep this file in the 'muted' list for a while. 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? That's for the result of call to RowSetMetadata::GetColumnBlocksById() at line 502. Line 38: #include "kudu/cfile/cfile_reader.h" > warning: #includes are not sorted properly [llvm-include-order] Done 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? The same story as with REPLICATE_SEQUENCE_OF_MESSAGES macro in raft_consensus_quorum-test.cc Line 62: using kudu::consensus::MaximumOpId; > warning: using decl 'MaximumOpId' is unused [misc-unused-using-decls] Done Line 63: using kudu::consensus::MinimumOpId; > warning: using decl 'MinimumOpId' is unused [misc-unused-using-decls] Done Line 64: using kudu::consensus::OpIdEquals; > warning: using decl 'OpIdEquals' is unused [misc-unused-using-decls] Done 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. Apparently, it wants to drop this due to a bug. It seems they have some provisions in the code or mappings that say to recommend dropping any <[x]stream> if <iostream> is included as well. 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 That's because IWYU wants to know about the components used in the macro. If not moving it down here, it wants forward declarations to be present prior to this macro. http://gerrit.cloudera.org:8080/#/c/8088/1/src/kudu/util/pb_util-test.cc File src/kudu/util/pb_util-test.cc: Line 19: #include <ostream> > warning: #includes are not sorted properly [llvm-include-order] Done Line 25: #include <google/protobuf/descriptor.h> > warning: #includes are not sorted properly [llvm-include-order] Done 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? It wants to drop it due to some bug. I don't know the details yet. -- 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: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
