You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Grant Henke (Code Review)" <ge...@cloudera.org> on 2018/02/08 18:13:17 UTC
[kudu-CR] Fix client comaptibility with gcc 4.4
Grant Henke has uploaded this change for review. ( http://gerrit.cloudera.org:8080/9257
Change subject: Fix client comaptibility with gcc 4.4
......................................................................
Fix client comaptibility with gcc 4.4
We expect the client lib to work with gcc 4.4 and C++98.
When the int128 type was added this broke compatibility
because __int128 support didn’t exist until gcc 4.6.
To work around this and still allow gcc 4.6+ and C++11
compilation to support int128, a preprocessor check
was added to define KUDU_INT128_SUPPORTED
which is used to exclude the incompatible client code.
Additionally the int128 insertion operator definitions
were moved to their own file to ensure they are not
required when working with Kudu’s int128 types.
This ensures there aren’t compatibily issues with clients
that define their own __int128 insertion operator.
Change-Id: I59b40a9718b321df1a5878160ac845d4cf3d9170
---
M src/kudu/client/scan_batch.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/value-internal.h
M src/kudu/client/value.cc
M src/kudu/client/value.h
M src/kudu/common/column_predicate.cc
M src/kudu/common/key_encoder.cc
M src/kudu/common/key_encoder.h
M src/kudu/common/key_util.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/util/CMakeLists.txt
M src/kudu/util/decimal_util.cc
M src/kudu/util/decimal_util.h
M src/kudu/util/int128-test.cc
M src/kudu/util/int128.h
R src/kudu/util/int128_util.h
19 files changed, 103 insertions(+), 24 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/57/9257/1
--
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: newchange
Gerrit-Change-Id: I59b40a9718b321df1a5878160ac845d4cf3d9170
Gerrit-Change-Number: 9257
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@gmail.com>
[kudu-CR] Fix client comaptibility with gcc 4.4
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9257
to look at the new patch set (#5).
Change subject: Fix client comaptibility with gcc 4.4
......................................................................
Fix client comaptibility with gcc 4.4
We expect the client lib to work with gcc 4.4 and C++98.
When the int128 type was added this broke compatibility
because __int128 support didn’t exist until gcc 4.6.
To work around this and still allow gcc 4.6+ and C++11
compilation to support int128, a preprocessor check
was added to define KUDU_INT128_SUPPORTED
which is used to exclude the incompatible client code.
Additionally the int128 insertion operator definitions
were moved to their own file to ensure they are not
required when working with Kudu’s int128 types.
This ensures there aren’t compatibily issues with clients
that define their own __int128 insertion operator.
Change-Id: I59b40a9718b321df1a5878160ac845d4cf3d9170
---
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/encoding-test.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/value.cc
M src/kudu/client/value.h
M src/kudu/common/key_encoder.h
M src/kudu/common/partial_row.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/int128-test.cc
M src/kudu/util/int128.h
R src/kudu/util/int128_util.h
12 files changed, 38 insertions(+), 19 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/57/9257/5
--
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: newpatchset
Gerrit-Change-Id: I59b40a9718b321df1a5878160ac845d4cf3d9170
Gerrit-Change-Number: 9257
Gerrit-PatchSet: 5
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Fix client comaptibility with gcc 4.4
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9257/4/src/kudu/util/int128.h
File src/kudu/util/int128.h:
http://gerrit.cloudera.org:8080/#/c/9257/4/src/kudu/util/int128.h@24
PS4, Line 24: defined(__clang__) || \
: (defined(__GNUC__) && \
: (__GNUC__ * 10000 + __GNUC_MINOR__ * 100) >= 40600)
this probably needs parens around the whole thing or else it would cause problems in an expression like:
#if KUDU_INT128_SUPPORTED && BLAH_BLAH
(due to operator precedence the && would apply only to the second half of the conjunction)
Alternatively change to:
#if ...
#define KUDU_INT128_SUPPORTED 1
#else
#define KUDU_INT128_SUPPORTED 0
#endif
--
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: 4
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 09 Feb 2018 19:40:42 +0000
Gerrit-HasComments: Yes
[kudu-CR] Fix client comaptibility with gcc 4.4
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9257
to look at the new patch set (#2).
Change subject: Fix client comaptibility with gcc 4.4
......................................................................
Fix client comaptibility with gcc 4.4
We expect the client lib to work with gcc 4.4 and C++98.
When the int128 type was added this broke compatibility
because __int128 support didn’t exist until gcc 4.6.
To work around this and still allow gcc 4.6+ and C++11
compilation to support int128, a preprocessor check
was added to define KUDU_INT128_SUPPORTED
which is used to exclude the incompatible client code.
Additionally the int128 insertion operator definitions
were moved to their own file to ensure they are not
required when working with Kudu’s int128 types.
This ensures there aren’t compatibily issues with clients
that define their own __int128 insertion operator.
Change-Id: I59b40a9718b321df1a5878160ac845d4cf3d9170
---
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/encoding-test.cc
M src/kudu/client/scan_batch.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/value-internal.h
M src/kudu/client/value.cc
M src/kudu/client/value.h
M src/kudu/common/column_predicate.cc
M src/kudu/common/key_encoder.cc
M src/kudu/common/key_encoder.h
M src/kudu/common/key_util.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/decimal_util.cc
M src/kudu/util/decimal_util.h
M src/kudu/util/int128-test.cc
M src/kudu/util/int128.h
R src/kudu/util/int128_util.h
22 files changed, 110 insertions(+), 26 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/57/9257/2
--
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: newpatchset
Gerrit-Change-Id: I59b40a9718b321df1a5878160ac845d4cf3d9170
Gerrit-Change-Number: 9257
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Fix client comaptibility with gcc 4.4
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9257 )
Change subject: Fix client comaptibility with gcc 4.4
......................................................................
Fix client comaptibility with gcc 4.4
We expect the client lib to work with gcc 4.4 and C++98.
When the int128 type was added this broke compatibility
because __int128 support didn’t exist until gcc 4.6.
To work around this and still allow gcc 4.6+ and C++11
compilation to support int128, a preprocessor check
was added to define KUDU_INT128_SUPPORTED
which is used to exclude the incompatible client code.
Additionally the int128 insertion operator definitions
were moved to their own file to ensure they are not
required when working with Kudu’s int128 types.
This ensures there aren’t compatibily issues with clients
that define their own __int128 insertion operator.
Change-Id: I59b40a9718b321df1a5878160ac845d4cf3d9170
Reviewed-on: http://gerrit.cloudera.org:8080/9257
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon <to...@apache.org>
---
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/encoding-test.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/value.cc
M src/kudu/client/value.h
M src/kudu/common/key_encoder.h
M src/kudu/common/partial_row.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/int128-test.cc
M src/kudu/util/int128.h
R src/kudu/util/int128_util.h
12 files changed, 38 insertions(+), 19 deletions(-)
Approvals:
Kudu Jenkins: Verified
Todd Lipcon: Looks good to me, approved
--
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: merged
Gerrit-Change-Id: I59b40a9718b321df1a5878160ac845d4cf3d9170
Gerrit-Change-Number: 9257
Gerrit-PatchSet: 6
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Fix client comaptibility with gcc 4.4
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 5: Code-Review+2
--
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: 5
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Sat, 10 Feb 2018 00:26:57 +0000
Gerrit-HasComments: No
[kudu-CR] Fix client comaptibility with gcc 4.4
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9257
to look at the new patch set (#4).
Change subject: Fix client comaptibility with gcc 4.4
......................................................................
Fix client comaptibility with gcc 4.4
We expect the client lib to work with gcc 4.4 and C++98.
When the int128 type was added this broke compatibility
because __int128 support didn’t exist until gcc 4.6.
To work around this and still allow gcc 4.6+ and C++11
compilation to support int128, a preprocessor check
was added to define KUDU_INT128_SUPPORTED
which is used to exclude the incompatible client code.
Additionally the int128 insertion operator definitions
were moved to their own file to ensure they are not
required when working with Kudu’s int128 types.
This ensures there aren’t compatibily issues with clients
that define their own __int128 insertion operator.
Change-Id: I59b40a9718b321df1a5878160ac845d4cf3d9170
---
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/encoding-test.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/value.cc
M src/kudu/client/value.h
M src/kudu/common/key_encoder.h
M src/kudu/common/partial_row.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/int128-test.cc
M src/kudu/util/int128.h
R src/kudu/util/int128_util.h
12 files changed, 34 insertions(+), 19 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/57/9257/4
--
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: newpatchset
Gerrit-Change-Id: I59b40a9718b321df1a5878160ac845d4cf3d9170
Gerrit-Change-Number: 9257
Gerrit-PatchSet: 4
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Fix client comaptibility with gcc 4.4
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9257
to look at the new patch set (#3).
Change subject: Fix client comaptibility with gcc 4.4
......................................................................
Fix client comaptibility with gcc 4.4
We expect the client lib to work with gcc 4.4 and C++98.
When the int128 type was added this broke compatibility
because __int128 support didn’t exist until gcc 4.6.
To work around this and still allow gcc 4.6+ and C++11
compilation to support int128, a preprocessor check
was added to define KUDU_INT128_SUPPORTED
which is used to exclude the incompatible client code.
Additionally the int128 insertion operator definitions
were moved to their own file to ensure they are not
required when working with Kudu’s int128 types.
This ensures there aren’t compatibily issues with clients
that define their own __int128 insertion operator.
Change-Id: I59b40a9718b321df1a5878160ac845d4cf3d9170
---
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/encoding-test.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/value.cc
M src/kudu/client/value.h
M src/kudu/common/key_encoder.h
M src/kudu/common/key_util.cc
M src/kudu/common/partial_row.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/int128-test.cc
M src/kudu/util/int128.h
R src/kudu/util/int128_util.h
13 files changed, 35 insertions(+), 19 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/57/9257/3
--
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: newpatchset
Gerrit-Change-Id: I59b40a9718b321df1a5878160ac845d4cf3d9170
Gerrit-Change-Number: 9257
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Fix client comaptibility with gcc 4.4
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 09 Feb 2018 01:09:14 +0000
Gerrit-HasComments: Yes
[kudu-CR] Fix client comaptibility with gcc 4.4
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/9257 )
Change subject: Fix client comaptibility with gcc 4.4
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9257/4/src/kudu/util/int128.h
File src/kudu/util/int128.h:
http://gerrit.cloudera.org:8080/#/c/9257/4/src/kudu/util/int128.h@24
PS4, Line 24: defined(__clang__) || \
: (defined(__GNUC__) && \
: (__GNUC__ * 10000 + __GNUC_MINOR__ * 100) >= 40600)
> this probably needs parens around the whole thing or else it would cause pr
Done
--
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: 4
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 09 Feb 2018 20:23:26 +0000
Gerrit-HasComments: Yes
[kudu-CR] Fix client comaptibility with gcc 4.4
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/9257 )
Change subject: Fix client comaptibility with gcc 4.4
......................................................................
Patch Set 2:
(2 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 l
Done
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
Done
--
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 <gr...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 09 Feb 2018 02:38:53 +0000
Gerrit-HasComments: Yes