Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ORC-552 fix c++ for centos6. #436

Closed
wants to merge 3 commits into from
Closed

Conversation

omalley
Copy link
Contributor

@omalley omalley commented Oct 18, 2019

Fix some compilation items on different compilers:

  • Add -Wno-c++2a-compat for new clang compilers.
  • Change the column printer parameter name to _buffer so that it doesn't hide the field.
  • Change the capacity parameter name to _capacity so that it doesn't hide the field.
  • Replace the emplace with insert so that it works on centos 6.
  • Replace the for in loop for centos 6.
  • Make TestColumnReader ignore parentheses warnings.
  • Make snappy use lib instead of lib64 for the library directory name for centos 8.

c++/src/Reader.cc Outdated Show resolved Hide resolved
CMakeLists.txt Outdated
set (WARN_FLAGS "${WARN_FLAGS} -Wconversion -Werror")
set (WARN_FLAGS "${WARN_FLAGS} -Wconversion")
if ((NOT CMAKE_CXX_COMPILER_VERSION STREQUAL "") AND
CMAKE_CXX_COMPILER_VERSION VERSION_GREATER "11.0")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does Clang have a 11.0 release?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, the problem was with the MacOS Clang, which reports itself as:

omalley@dhcp-10-16-1-45> clang --version
Apple clang version 11.0.0 (clang-1100.0.33.8)
Target: x86_64-apple-darwin18.7.0
Thread model: posix
InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin

I see your point that there isn't an upstream clang 11. I'm not sure what Apple is doing with their version numbers or the right way to detect it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think Apple uses the XCode version.
Instead of CMAKE_CXX_COMPILER_VERSION should we use
"${COMPILER_VERSION_FULL}" MATCHES ".*clang-11"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used CMAKE_HOST_APPLE and the CMAKE_CXX_COMPILER_VERSION > 11. No doubt when some other OS is using clang 8?, we'll need to generalize it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On MacOS Mojave (10.14), COMPILER_VERSION_FULL is undefined on cmake...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see now that they are using the compiler itself to get that value. Current XCode doesn't match their patterns. :(

Copy link
Contributor

@majetideepak majetideepak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 LGTM

@omalley omalley closed this in fed27e3 Oct 21, 2019
omalley added a commit that referenced this pull request Oct 22, 2019
Fixes #436

Signed-off-by: Owen O'Malley <omalley@apache.org>
omalley added a commit to omalley/orc that referenced this pull request Oct 23, 2019
Fixes apache#436

Signed-off-by: Owen O'Malley <omalley@apache.org>
omalley added a commit to omalley/orc that referenced this pull request Dec 6, 2019
Fixes apache#436

Signed-off-by: Owen O'Malley <omalley@apache.org>
omalley added a commit to omalley/orc that referenced this pull request Dec 6, 2019
Fixes apache#436

Signed-off-by: Owen O'Malley <omalley@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants