Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9257 )
Change subject: Fix client comaptibility with gcc 4.4 ...................................................................... Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/9257/2/src/kudu/common/key_encoder.cc File src/kudu/common/key_encoder.cc: http://gerrit.cloudera.org:8080/#/c/9257/2/src/kudu/common/key_encoder.cc@58 PS2, Line 58: #ifdef KUDU_INT128_SUPPORTED for these changes in .cc files is this required? We only build the client lib with compilers that support int128, so we should only need the changes in the headers that are included by end-user code http://gerrit.cloudera.org:8080/#/c/9257/2/src/kudu/common/types.h File src/kudu/common/types.h: http://gerrit.cloudera.org:8080/#/c/9257/2/src/kudu/common/types.h@327 PS2, Line 327: #ifdef KUDU_INT128_SUPPORTED This header isn't part of the client library so I don't think it should be necessary (you can check the install() directives in client/CMakeLists.txt to see which ones get installed) http://gerrit.cloudera.org:8080/#/c/9257/2/src/kudu/util/int128.h File src/kudu/util/int128.h: http://gerrit.cloudera.org:8080/#/c/9257/2/src/kudu/util/int128.h@26 PS2, Line 26: #define KUDU_INT128_SUPPORTED it might be safer to #define this to either 0 or 1 and then use #if KUDU_INT128_SUPPORTED everywehre instead of ifdef. That way you won't accidentally forgot to include int128.h and presume that it's not supported -- To view, visit http://gerrit.cloudera.org:8080/9257 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I59b40a9718b321df1a5878160ac845d4cf3d9170 Gerrit-Change-Number: 9257 Gerrit-PatchSet: 2 Gerrit-Owner: Grant Henke <granthe...@gmail.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Fri, 09 Feb 2018 01:09:14 +0000 Gerrit-HasComments: Yes
