Alexey Serbin has posted comments on this change. ( )

Change subject: iwyu: codebase-wide fixes based on libcpp

Patch Set 3:

File src/kudu/client/write_op.h:
PS3, Line 22: IWYU pragma: no_include <memory>
nit: could you add a comment explaining why this is necessary?
File src/kudu/common/
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?
File src/kudu/common/
PS3, Line 21: #include <string> // IWYU pragma: keep
Interesting: why is it needed?  I don't see std::string-related code in this 
File src/kudu/rpc/
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?
File src/kudu/security/openssl_util.h:
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?
File src/kudu/security/
PS3, Line 36: #include "kudu/util/debug/leakcheck_disabler.h" // IWYU pragma: 
nit: would be a better optio putting this under '#if OPENSSL_VERSION_NUMBER < 
0x10100000L' ... #endif ?
PS3, Line 38: #include "kudu/util/mutex.h" // IWYU pragma: keep
File src/kudu/tablet/
PS3, Line 20: #include <string.h>
#include <cstring>

File src/kudu/util/
PS3, Line 33: #include <limits.h>
nit: would <climits> work here?
File src/kudu/util/
PS3, Line 10: #include <limits.h>
nit: would <climits> work here?
File src/kudu/util/status.h:
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
To unsubscribe, visit

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic248ba1511347d79cc6ea38140de888e5ac13354
Gerrit-Change-Number: 15543
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo <>
Gerrit-Reviewer: Adar Dembo <>
Gerrit-Reviewer: Alexey Serbin <>
Gerrit-Reviewer: Andrew Wong <>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 26 Mar 2020 02:24:03 +0000
Gerrit-HasComments: Yes

Reply via email to