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