Dan Burkert has posted comments on this change.

Change subject: WIP: [iwyu] first pass
......................................................................


Patch Set 11:

(19 comments)

Thanks for banging on this some more, will be great to have with the automated 
checking.  I only got through about 40%, but I saw a lot of repeated patterns 
that are kind of suspicious.  Basically most uses of the pragmas surprised me.  
Are these pragmas indicating issues with IWYU, or with our includes?  In 
particular logging.h, port.h, and boost headers keep reappearing.

http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/benchmarks/rle.cc
File src/kudu/benchmarks/rle.cc:

Line 28: #include <glog/logging.h> // IWYU pragma: keep
Might be good to make a comment in the commit message about these different 
pragma types, e.g. what are they doing, and is it a long term or temporary 
solution?


http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/benchmarks/tpch/line_item_tsv_importer.h
File src/kudu/benchmarks/tpch/line_item_tsv_importer.h:

Line 21: //#include <ostream>
leftover?


http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/benchmarks/tpch/rpc_line_item_dao-test.cc
File src/kudu/benchmarks/tpch/rpc_line_item_dao-test.cc:

Line 18: #include <sys/types.h>
sys/types.h and gutil/port.h seem suspicious to me.  I don't see anything in 
this file which might need those.


http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/benchmarks/tpch/rpc_line_item_dao.h
File src/kudu/benchmarks/tpch/rpc_line_item_dao.h:

Line 24: #include <boost/function.hpp> // IWYU pragma: keep
Could you forward declare boost::function instead?


http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/benchmarks/tpch/tpch-schemas.h
File src/kudu/benchmarks/tpch/tpch-schemas.h:

Line 25: #include <glog/logging.h> // for CHECK_OK to be picked up in status.h
Perhaps we could add this include inside the '#ifdef KUDU_HEADERS_NO_STUBS' in 
status.h instead of including it in all the call sites?  E.g. right here: 
https://github.com/apache/kudu/blob/d1f53cc323d5d07e79304765fe171f535c1d548a/src/kudu/util/status.h#L22


http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/benchmarks/tpch/tpch1.cc
File src/kudu/benchmarks/tpch/tpch1.cc:

Line 60: #include <sys/types.h>
Another un-obvious one.


http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/client/scan_predicate.cc
File src/kudu/client/scan_predicate.cc:

Line 48: 
extra


http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/client/table_alterer-internal.h
File src/kudu/client/table_alterer-internal.h:

Line 24: #include <boost/optional/optional.hpp>
Hm, did plain boost/optional.hpp result in a warning?


http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/client/table_creator-internal.h
File src/kudu/client/table_creator-internal.h:

Line 24: #include <boost/optional.hpp> // IWYU pragma: keep
I think this could be optional_fwd.hpp


http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/client/write_op.h
File src/kudu/client/write_op.h:

Line 20: // NOTE: using stdint.h instead of cstdint because this file is 
supposed
client_samples-test covers this, right?  Probably no need to comment this and 
client.h if we do.  These headers are meant to be consumed externally and I 
think it may be a bit confusing without context.


http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/clock/logical_clock.cc
File src/kudu/clock/logical_clock.cc:

Line 73:                                     const MonoTime&) {
Don't we usually do something like /* deadline */?


http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/codegen/row_projector.h
File src/kudu/codegen/row_projector.h:

Line 35: // IWYU pragma: no_include "kudu/gutil/int128.h"
These keep showing up over and over, does that indicate some kind of issue with 
our organization?


http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/common/column_predicate.cc
File src/kudu/common/column_predicate.cc:

Line 34: 
extra


http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/common/partition_pruner.h
File src/kudu/common/partition_pruner.h:

Line 31: class Partition;        // IWYU pragma: keep
I think these forward declares, or perhaps the include of partition.h could be 
removed.


http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/common/row_changelist.cc
File src/kudu/common/row_changelist.cc:

Line 31: #include "kudu/gutil/port.h"
This one keeps showing up, not sure what it's being used for.


http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/common/row_changelist.h
File src/kudu/common/row_changelist.h:

Line 41: class faststring; // IWYU pragma: keep
I Don't see why the pragma would be necessary with the fwd declare?


http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/common/scan_spec.cc
File src/kudu/common/scan_spec.cc:

Line 41: // IWYU pragma: no_include <boost/core/explicit_operator_bool.hpp>
These are the only two uses of boost in the file, that's suspicious.


http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/common/schema-test.cc
File src/kudu/common/schema-test.cc:

Line 18: 
I think it's in our style to include the tested header first.


http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/common/schema.h
File src/kudu/common/schema.h:

Line 48: // IWYU pragma: no_include "kudu/gutil/int128.h"
Another suspicious pair that keeps showing up.


-- 
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: 11
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: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes

Reply via email to