Alexey Serbin has posted comments on this change. Change subject: [iwyu] first pass ......................................................................
Patch Set 18: (41 comments) Thank you for the thorough review! 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. Done 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 Done 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? I thought so as well, but it failed to compile: /data/8/aserbin/Projects/kudu/src/kudu/gutil/gscoped_ptr.h:144:36: error: invalid application of 'sizeof' to an incomplete type 'kudu::client::KuduWriteOperation' enum { type_must_be_complete = sizeof(T) }; That's why I added the pragma. I think it would be OK with the unique_ptr, but gscoped_ptr has a special compile-time check in its deleter. Maybe, the newer code of gutil library has this corrected? 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 Good catch! It didn't hurt in that case -- IWYU is intelligent enough to track that down and I didn't see it suggesting this header instead of <memory> in that case, but sure -- that's not a good idea to export <memory> symbols from here. I'll remove this. 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. Yep, you are right. I mistakenly moved it down. Will fix. 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? Huh, it seemed to me I added that because compilation was broken if doing forward declaration -- that's why I added the pragma. But now it passes. 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 ^ good catch -- indeed, i unintentionally removed that. it's back now. 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? Yes, for some reason IWYU prefers this to optional_fwd.hpp. Probably, once I'm more familiar with IWYU code I can figure out why and try to fix it. But as of now, to keep the number of pragmas as less as possible, I decided just to please IWYU with adding what it asked for. 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 Done 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 Done 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 us That's because the boost mappings put the smart_ptr/detail/spinlock.hpp header into the 'private' set, recommending to add the 'public' boost/smart_ptr/shared_ptr.hpp I think we could do one of the following: 1) return detail/spinlock.hpp back and add the file into the 'mute' list 2) return detail/spinlock.hpp back and update the boost mappings. 3) leave it as is I think option 1 is the best in the short run. 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 Done 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 Done 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 Done 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 Yep: as already mentioned, I tried that but IWYU prefers this way. I suggest to leave it as is for now, and then we can clarify on this a little bit later in the background. 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? Done 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 same -- to keep IWYU happy I propose to use this way until we clarify on this. 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 I don't think so -- one of the classes below contains a member of boost::function<void(const Status&)> class. 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 same -- I suggest to keep IWYU happy by this way of forward declaration for a while. 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 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 ditto :) 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 Right, that's left-over from my editing. 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 same -- let's keep IWYU happy by this way of forward declaration, and I will investigate on how to do that in a proper way in the background. 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 I also thought that first, but it seems it's necessary to have more information on the type of std::unordered_map members, otherwise the compiler gives an error: In file included from /data/8/aserbin/Projects/kudu/src/kudu/tablet/delta_stats.cc:17: In file included from /data/8/aserbin/Projects/kudu/src/kudu/tablet/delta_stats.h:23: In file included from /opt/rh/devtoolset-3/root/usr/lib/gcc/x86_64-redhat-linux/4.9.2/../../../../include/c++/4.9.2/unordered_map:47: In file included from /opt/rh/devtoolset-3/root/usr/lib/gcc/x86_64-redhat-linux/4.9.2/../../../../include/c++/4.9.2/bits/hashtable.h:35: /opt/rh/devtoolset-3/root/usr/lib/gcc/x86_64-redhat-linux/4.9.2/../../../../include/c++/4.9.2/bits/hashtable_policy.h:85:11: error: implicit instantiation of undefined template 'std::hash<kudu::ColumnId>' noexcept(declval<const _Hash&>()(declval<const _Key&>()))> ^ 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? That's the issue -- the IWYU does not think forward declaration is enough (BTW, thank you for pointing that it's missing -- I'll add it) and suggests to include the header. 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 ditto about keeping IWYU happy this unusual way. 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 Done 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 Done 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 &st Ah, yes. It seems I just added it in to keep things simpler. Is it OK to keep it in here or you want me to publish this as a separate changelist? 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 Yep, that would be the right way doing this. However, it's the same as with boost::optional -- IWYU is not happy with those dedicated fwd headers and wants this simple declaration. I hope we will be able to address this in the nearest future, but I suggest we keep it as is for now. http://gerrit.cloudera.org:8080/#/c/4738/18/src/kudu/util/env.cc File src/kudu/util/env.cc: Line 7: > extra Done 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 Done 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 Done 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 Done 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. Done Line 199: << "Expected: " << HexDump(kudu::Slice(expected_encoding, expected_len)) << "\n" > this shouldn't be necessary Done 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. That's for PREDICT_FALSE, not int types. http://gerrit.cloudera.org:8080/#/c/4738/18/src/kudu/util/thread.cc File src/kudu/util/thread.cc: Line 63: > extra Done 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 yep, that's another manifestation of the boost mappings: <boost/smart_ptr/detail/yield_k.hpp> is declared for 'private' use there, and <boost/smart_ptr/shared_ptr.hpp> is the file which IWYU saw was including that 'private' file. 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 Right. Since IWYU isn't happy with function_fwd.hpp, let's keep the in-file forward declaration for boost::function. Line 30: #include "kudu/gutil/callback_forward.h" // IWYU pragma: keep > shoulldn't be necessary to have both, right? Woops. Good catch! Of course. It seems there have been multiple iterations, so eventually it ended up like this. IWYU is not happy with forward declaration of the Closure class (whether via the callback_forward.h or just in-file), and it asks for kudu/gutil/callback.h. Another point to clarify in the long-run. -- 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 <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
