Dan Burkert has posted comments on this change. Change subject: [iwyu] first pass ......................................................................
Patch Set 18: (41 comments) Good job on trimming the pragmas, looking much better now. http://gerrit.cloudera.org:8080/#/c/4738/18/src/kudu/client/client-internal.cc File src/kudu/client/client-internal.cc: Line 31: #include <boost/function.hpp> // IWYU pragma: keep I think function could be forward declared. http://gerrit.cloudera.org:8080/#/c/4738/18/src/kudu/client/client-unittest.cc File src/kudu/client/client-unittest.cc: Line 36: extra space http://gerrit.cloudera.org:8080/#/c/4738/18/src/kudu/client/error-internal.cc File src/kudu/client/error-internal.cc: Line 23: #include "kudu/client/write_op.h" // IWYU pragma: keep KuduWriteOperation could perhaps be forward declared? http://gerrit.cloudera.org:8080/#/c/4738/18/src/kudu/client/shared_ptr.h File src/kudu/client/shared_ptr.h: Line 42: #include <memory> // IWYU pragma: export I don't think this pragma is correct (or the one below): this file is only about defining the kudu::client::sp::* symbols. If a file is using std::shared_ptr, i'd expect it to have to include <memory> normally. http://gerrit.cloudera.org:8080/#/c/4738/18/src/kudu/codegen/jit_wrapper.cc File src/kudu/codegen/jit_wrapper.cc: Line 22: #include "kudu/codegen/jit_wrapper.h" I think it was correct to list this one first. http://gerrit.cloudera.org:8080/#/c/4738/18/src/kudu/common/key_encoder.cc File src/kudu/common/key_encoder.cc: Line 25: #include "kudu/util/faststring.h" // IWYU pragma: keep forward declare? http://gerrit.cloudera.org:8080/#/c/4738/18/src/kudu/common/schema.h File src/kudu/common/schema.h: Line 1: // or more contributor license agreements. See the NOTICE file Typo ^ http://gerrit.cloudera.org:8080/#/c/4738/18/src/kudu/common/wire_protocol.h File src/kudu/common/wire_protocol.h: Line 29: class optional; I'm curious - is this preferred to optional_fwd.hpp? http://gerrit.cloudera.org:8080/#/c/4738/18/src/kudu/consensus/mt-log-test.cc File src/kudu/consensus/mt-log-test.cc: Line 33: extra space http://gerrit.cloudera.org:8080/#/c/4738/18/src/kudu/consensus/raft_consensus.cc File src/kudu/consensus/raft_consensus.cc: Line 74: extra space http://gerrit.cloudera.org:8080/#/c/4738/18/src/kudu/experiments/rwlock-perf.cc File src/kudu/experiments/rwlock-perf.cc: Line 29: #include <boost/smart_ptr/shared_ptr.hpp> This one's suspicious - I think we've gotten rid of all boost shared_ptr usage? http://gerrit.cloudera.org:8080/#/c/4738/18/src/kudu/fs/block_manager.cc File src/kudu/fs/block_manager.cc: Line 18: extra space http://gerrit.cloudera.org:8080/#/c/4738/18/src/kudu/fs/data_dirs.h File src/kudu/fs/data_dirs.h: Line 33: #include "kudu/gutil/callback_forward.h" // IWYU pragma: keep I don't think it needs callback.h and callback_forward.h http://gerrit.cloudera.org:8080/#/c/4738/18/src/kudu/gutil/strings/join.h File src/kudu/gutil/strings/join.h: Line 19: // extra line http://gerrit.cloudera.org:8080/#/c/4738/18/src/kudu/integration-tests/cluster_itest_util.h File src/kudu/integration-tests/cluster_itest_util.h: Line 49: class optional; same - consider optional_fwd.hpp http://gerrit.cloudera.org:8080/#/c/4738/18/src/kudu/integration-tests/test_workload.cc File src/kudu/integration-tests/test_workload.cc: Line 37: //#include "kudu/integration-tests/internal_mini_cluster.h" leftover? http://gerrit.cloudera.org:8080/#/c/4738/18/src/kudu/master/catalog_manager.h File src/kudu/master/catalog_manager.h: Line 53: class optional; same - consider optional_fwd.hpp http://gerrit.cloudera.org:8080/#/c/4738/18/src/kudu/rpc/reactor.h File src/kudu/rpc/reactor.h: Line 27: #include <boost/function.hpp> // IWYU pragma: keep might be possible to forward declare http://gerrit.cloudera.org:8080/#/c/4738/18/src/kudu/rpc/service_queue.h File src/kudu/rpc/service_queue.h: Line 37: class optional; same -condisder optional_fwd.hpp http://gerrit.cloudera.org:8080/#/c/4738/18/src/kudu/security/cert.h File src/kudu/security/cert.h: Line 32: template <class T> same http://gerrit.cloudera.org:8080/#/c/4738/18/src/kudu/security/init.h File src/kudu/security/init.h: Line 22: template <class T> same http://gerrit.cloudera.org:8080/#/c/4738/18/src/kudu/server/generic_service.cc File src/kudu/server/generic_service.cc: Line 26: #include <glog/logging.h> // for LOG, LogMessage, COMPACT_GOO... I think this is self explanatory http://gerrit.cloudera.org:8080/#/c/4738/18/src/kudu/tablet/cfile_set.h File src/kudu/tablet/cfile_set.h: Line 45: class optional; same http://gerrit.cloudera.org:8080/#/c/4738/18/src/kudu/tablet/delta_stats.h File src/kudu/tablet/delta_stats.h: Line 25: #include "kudu/common/schema.h" // IWYU pragma: keep Is this just for ColumnId? Might be able to forward declare http://gerrit.cloudera.org:8080/#/c/4738/18/src/kudu/tablet/rowset.h File src/kudu/tablet/rowset.h: Line 41: // IWYU pragma: no_include "kudu/util/monotime.h" Could this be replaced with a forward declaration of MonoTime? http://gerrit.cloudera.org:8080/#/c/4738/18/src/kudu/tablet/tablet_metadata.h File src/kudu/tablet/tablet_metadata.h: Line 44: template <class T> same http://gerrit.cloudera.org:8080/#/c/4738/18/src/kudu/tablet/transactions/alter_schema_transaction.cc File src/kudu/tablet/transactions/alter_schema_transaction.cc: Line 73: AlterSchemaTransaction::AlterSchemaTransaction(std::unique_ptr<AlterSchemaTransactionState> state, I don't think this is necessary http://gerrit.cloudera.org:8080/#/c/4738/18/src/kudu/tablet/transactions/write_transaction.cc File src/kudu/tablet/transactions/write_transaction.cc: Line 78: extra space http://gerrit.cloudera.org:8080/#/c/4738/18/src/kudu/tools/ksck.h File src/kudu/tools/ksck.h: Line 387: std::ostream* out = nullptr); is this a functional change? Seemed simpler before with the default of &std::out. I'm +1 on moving the impl, though. http://gerrit.cloudera.org:8080/#/c/4738/18/src/kudu/tools/tool_action_common.cc File src/kudu/tools/tool_action_common.cc: Line 92: class function; might be worth creating our own function_fwd.hpp or similar because this is pretty common. http://gerrit.cloudera.org:8080/#/c/4738/18/src/kudu/util/env.cc File src/kudu/util/env.cc: Line 7: extra http://gerrit.cloudera.org:8080/#/c/4738/18/src/kudu/util/env_posix.cc File src/kudu/util/env_posix.cc: Line 66: // IWYU pragma: no_include <asm/int-ll64.h> consider moving these up with the other sytem headers http://gerrit.cloudera.org:8080/#/c/4738/18/src/kudu/util/make_shared.h File src/kudu/util/make_shared.h: Line 22: //#include <ext/new_allocator.h> // IWYU pragma: export leftover http://gerrit.cloudera.org:8080/#/c/4738/18/src/kudu/util/memory/arena.h File src/kudu/util/memory/arena.h: Line 41: //#include "kudu/gutil/logging-inl.h" leftover http://gerrit.cloudera.org:8080/#/c/4738/18/src/kudu/util/rle-test.cc File src/kudu/util/rle-test.cc: Line 27: #include "kudu/gutil/mathlimits.h" Probably best to retain the space here. Line 199: << "Expected: " << HexDump(kudu::Slice(expected_encoding, expected_len)) << "\n" this shouldn't be necessary http://gerrit.cloudera.org:8080/#/c/4738/18/src/kudu/util/slice.cc File src/kudu/util/slice.cc: Line 22: #include "kudu/gutil/port.h" I'm still confused about where all these port.h instances are coming from. If it's int types, should we instead be including <cstdint>? There are many many additions of port.h. http://gerrit.cloudera.org:8080/#/c/4738/18/src/kudu/util/thread.cc File src/kudu/util/thread.cc: Line 63: extra http://gerrit.cloudera.org:8080/#/c/4738/18/src/kudu/util/threadpool-test.cc File src/kudu/util/threadpool-test.cc: Line 33: #include <boost/smart_ptr/shared_ptr.hpp> Another one suspicious boost shared_ptr instance http://gerrit.cloudera.org:8080/#/c/4738/18/src/kudu/util/threadpool.h File src/kudu/util/threadpool.h: Line 26: #include <boost/function.hpp> // IWYU pragma: keep shouldn't be necessary to include + forward decl Line 30: #include "kudu/gutil/callback_forward.h" // IWYU pragma: keep shoulldn't be necessary to have both, right? -- To view, visit http://gerrit.cloudera.org:8080/4738 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6923091be36a1c03fac34dd73e6b17e17ac9ceaa Gerrit-PatchSet: 18 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Jean-Daniel Cryans <jdcry...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
