Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15543 )
Change subject: iwyu: codebase-wide fixes based on libcpp ...................................................................... Patch Set 3: (11 comments) http://gerrit.cloudera.org:8080/#/c/15543/3/src/kudu/client/write_op.h File src/kudu/client/write_op.h: http://gerrit.cloudera.org:8080/#/c/15543/3/src/kudu/client/write_op.h@22 PS3, Line 22: IWYU pragma: no_include <memory> nit: could you add a comment explaining why this is necessary? http://gerrit.cloudera.org:8080/#/c/15543/3/src/kudu/common/encoded_key-test.cc File src/kudu/common/encoded_key-test.cc: http://gerrit.cloudera.org:8080/#/c/15543/3/src/kudu/common/encoded_key-test.cc@43 PS3, Line 43: namespace kudu { : class EncodedKeyTest; : class EncodedKeyTest_TestConstructFromEncodedString_Test; : class EncodedKeyTest_TestDecodeCompoundKeys_Test; : class EncodedKeyTest_TestDecodeSimpleKeys_Test; : } // namespace kudu Maybe, splitting the declaration and implementation of EncodedKeyTest and defining those macros after the declaration would be a better option? http://gerrit.cloudera.org:8080/#/c/15543/3/src/kudu/common/key_encoder.cc File src/kudu/common/key_encoder.cc: http://gerrit.cloudera.org:8080/#/c/15543/3/src/kudu/common/key_encoder.cc@21 PS3, Line 21: #include <string> // IWYU pragma: keep Interesting: why is it needed? I don't see std::string-related code in this file. http://gerrit.cloudera.org:8080/#/c/15543/3/src/kudu/rpc/negotiation-test.cc File src/kudu/rpc/negotiation-test.cc: http://gerrit.cloudera.org:8080/#/c/15543/3/src/kudu/rpc/negotiation-test.cc@34 PS3, Line 34: #include <krb5/krb5.h> // IWYU pragma: keep Interesting -- previously it was not in this file. If it was able to compile without this header file, why to keep it, especially with IWYU's pragma? http://gerrit.cloudera.org:8080/#/c/15543/3/src/kudu/security/openssl_util.h File src/kudu/security/openssl_util.h: http://gerrit.cloudera.org:8080/#/c/15543/3/src/kudu/security/openssl_util.h@35 PS3, Line 35: namespace kudu { : namespace security { : namespace internal { : struct ScopedCheckNoPendingSSLErrors; : } // namespace internal : } // namespace security : } // namespace kudu nit: maybe, moving the definition of the ScopedCheckNoPendingSSLErrors structure to the top would be a better option? http://gerrit.cloudera.org:8080/#/c/15543/3/src/kudu/security/openssl_util.cc File src/kudu/security/openssl_util.cc: http://gerrit.cloudera.org:8080/#/c/15543/3/src/kudu/security/openssl_util.cc@36 PS3, Line 36: #include "kudu/util/debug/leakcheck_disabler.h" // IWYU pragma: keep nit: would be a better optio putting this under '#if OPENSSL_VERSION_NUMBER < 0x10100000L' ... #endif ? http://gerrit.cloudera.org:8080/#/c/15543/3/src/kudu/security/openssl_util.cc@38 PS3, Line 38: #include "kudu/util/mutex.h" // IWYU pragma: keep ditto http://gerrit.cloudera.org:8080/#/c/15543/3/src/kudu/tablet/rowset_tree-test.cc File src/kudu/tablet/rowset_tree-test.cc: http://gerrit.cloudera.org:8080/#/c/15543/3/src/kudu/tablet/rowset_tree-test.cc@20 PS3, Line 20: #include <string.h> #include <cstring> ? http://gerrit.cloudera.org:8080/#/c/15543/3/src/kudu/util/env-test.cc File src/kudu/util/env-test.cc: http://gerrit.cloudera.org:8080/#/c/15543/3/src/kudu/util/env-test.cc@33 PS3, Line 33: #include <limits.h> nit: would <climits> work here? http://gerrit.cloudera.org:8080/#/c/15543/3/src/kudu/util/env_posix.cc File src/kudu/util/env_posix.cc: http://gerrit.cloudera.org:8080/#/c/15543/3/src/kudu/util/env_posix.cc@10 PS3, Line 10: #include <limits.h> nit: would <climits> work here? http://gerrit.cloudera.org:8080/#/c/15543/3/src/kudu/util/status.h File src/kudu/util/status.h: http://gerrit.cloudera.org:8080/#/c/15543/3/src/kudu/util/status.h@24 PS3, Line 24: // IWYU pragma: no_include <glog/logging.h> > Why don't we need this? Don't we actually pull LOG() from here? Probably, it's worth adding a small comment explaining why this is needed here or remove this at all? -- To view, visit http://gerrit.cloudera.org:8080/15543 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic248ba1511347d79cc6ea38140de888e5ac13354 Gerrit-Change-Number: 15543 Gerrit-PatchSet: 3 Gerrit-Owner: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 26 Mar 2020 02:24:03 +0000 Gerrit-HasComments: Yes
