Alexey Serbin has posted comments on this change.

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


Patch Set 12:

(21 comments)

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>
> Might be good to make a comment in the commit message about these different
Good idea.  I hope keeping these pragmas is a temporary solution until some 
bugs in the include-what-you-use tool are fixed or we find some other way of 
fixing it.


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 <vector>
> leftover?
Done


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 <algorithm>
> sys/types.h and gutil/port.h seem suspicious to me.  I don't see anything i
Yep, that's one of those recommendation the IWYU tools gave.  As I already 
mentioned -- it's necessary to validate its suggestions for every time, and 
that takes a lot of time.

Here it suggested to include <sys/types.h> for int64_t.  Apparently, it should 
have been <cstdint> or <stdint.h> instead.


Line 18: #include <algorithm>
> Perhaps sys/types.h is for the int64_t type?
Right.  Now it's fixed.


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 "kudu/client/client.h"
> Could you forward declare boost::function instead?
Good idea.  However, IWYU suggested this:

kudu/src/kudu/benchmarks/tpch/rpc_line_item_dao.h should add these lines:
#include <boost/function/function_template.hpp>  // for function
namespace boost { template <typename Signature> class function; }

Adding only the forward declaration would be enough.  But if so, then IWYU 
complained continued to complain and I decided to just add <boost/function.hpp>.

Also, it seems the previous version of the IWYU tools gave a different 
suggestions, not matching with those that current does (current is of v0.8).

I'll update this to keep the forward declaration and adding a pragma for not 
suggesting to include <boost/function/function_template.hpp>


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 "kudu/client/schema.h"
> Perhaps we could add this include inside the '#ifdef KUDU_HEADERS_NO_STUBS'
Yep, this is kind of strange left-over from my earlier experiments, and it 
seems I forgot to address it.

Thank you for pointing at it this time as well -- I'll address this.


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

Line 60: #include <cstdint>
> Another un-obvious one.
That was a suggestion from IWYU for int32_t.  Apparently, it should be covered 
by <cstdint>.  I'll check what the current 0.8 version suggests.


Line 60: #include <cstdint>
> Probably int32_t (see struct Result).
yep, that's true


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

Line 48: 
> extra
Done


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?
Yes, it did.  That's why I updated this.  We can amend this using our own 
mappings for the boost library.  Right now I'm using the 'standard' mappings 
from the IWYU source tree.


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/optional.hpp>
> I think this could be optional_fwd.hpp
The class has a member of boost::optional type, so I don't think forward 
declaration would be feasible here.

Yep -- the compiler output an error if trying to use the forward declaration.

IWYU tool suggested to use <boost/optional/optional.h>, which seems good enough.


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 a
Done


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 */?
Done


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

Line 35: #include "kudu/gutil/ref_counted.h"
> These keep showing up over and over, does that indicate some kind of issue 
I think this manifests a bug in the include-what-you-use tool, not with our 
header files organization.

It's more apparent when you see the suggestions that IWYU outputs:

<abs_path>/row_projector.h should add these lines:
...
#include "kudu/gutil/int128.h"                 // for ostream
...


I.e., the include-what-you-use tools suggests to include the 
"kudu/gutil/int128.h" header for std::ostream!  Apparently, that's incorrect.

As for the "kudu/util/logging.h" -- for some reason, the tool suggests to 
include it (instead of <glog/logging.h>) for DCHECK_EQ, CHECK_EQ, etc.  That's 
not correct.


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

Line 34: using std::move;
> extra
Done


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
Done


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.
types.h is usually for for TypeInfo and port.h is for unused().  Not sure it's 
needed, but the tool recommends to include them.


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

Line 41: 
> I Don't see why the pragma would be necessary with the fwd declare?
Because IWYU recommends including the kudu/util/faststring.h" header here 
instead, however, that's not necessary.


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.
I decided to remove this for a while and put the file into the 'muted' list.


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.
Done


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

Line 48: // Check that two schemas are equal, yielding a useful error message 
in the case that
> Another suspicious pair that keeps showing up.
That's clarified now: the problem was in presence of 'using' directives in some 
of Kudu header files.


-- 
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: 12
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

Reply via email to