Replace cl.hpp with cl2.hpp

[9prady9]

• Add OpenCL-CLHPP repository as external project and use cl2.hpp header instead of current cl.hpp header.
• Provide a cmake build option USE_SYSTEM_CL2HPP to use headers installed on the system. By default this value shall be OFF which results in download and generation of the headers from OpenCL-CLHPP repository.

This change is

[9prady9]

I have built the changes on the following compiler configuration

jesse:build arrayfire$ /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ --version
Apple LLVM version 6.1.0 (clang-602.0.49) (based on LLVM 3.6.0svn)
Target: x86_64-apple-darwin14.5.0
Thread model: posix
jesse:build arrayfire$ /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/cc --version
Apple LLVM version 6.1.0 (clang-602.0.49) (based on LLVM 3.6.0svn)
Target: x86_64-apple-darwin14.5.0
Thread model: posix

ProductName:    Mac OS X
ProductVersion: 10.10.5

and the changes built successfully.

[pavanky]

@kknox looks like travis is still on Mavericks which seems to be the cause of the failure.

We can hook up clSparse into arrayfire's jenkins for now if you are interested.

[pavanky]

This PR is still missing option to use cl2.hpp present on the system. Once this is done, this can remove the need for #138 and resolve #162

[9prady9]

I have added option to use cl2.hpp present on the system.

[kknox]

Reviewed 18 of 24 files at r1, 7 of 7 files at r2.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

.gitignore, line 34 [r2] (raw file):

# temp build file locations
build/

This is mostly a pet peeve of mine, but I do not like any generated files to be mixed in with the source, even if it's whitelisted in the .gitignore. I feel these are the types of exceptions that belong in a fork, not in upstream. I can't find below where you generate code in 'builld/', so this may be moot. Can you revert this change?

---

samples/sample-axpy.cpp, line 28 [r2] (raw file):

#define CL_HPP_MINIMUM_OPENCL_VERSION BUILD_CLVERSION
#define CL_HPP_TARGET_OPENCL_VERSION BUILD_CLVERSION
#include <CL/cl2.hpp>

I really like that the system specific pre-processor guards can be eliminated here. Thanks for this simplification!

---

src/clsparseTimer/CMakeLists.txt, line 65 [r2] (raw file):

if(${UNIX})
    ADD_DEFINITIONS(-fvisibility=hidden)
endif(${UNIX})

Why is this necessary? I would prefer to use the standard add_compiler_export_flags? I see above in a comment that you say it's deprecated? Can you provide material that I can read?

---

src/library/CMakeLists.txt, line 254 [r2] (raw file):

if(${UNIX})
    ADD_DEFINITIONS(-fvisibility=hidden)
endif(${UNIX})

Why is this necessary? I would prefer to use the standard add_compiler_export_flags?

---

Comments from Reviewable

[kknox]

According to this travis page, we can opt-in to a more recent version of mac osx. Can you update the .travis.yml file on your next commit to include osx_image: xcode8

[pavanky]

This is mostly a pet peeve of mine, but I do not like any generated files to be mixed in with the source, even if it's whitelisted in the .gitignore. I feel these are the types of exceptions that belong in a fork, not in upstream. I can't find below where you generate code in 'builld/', so this may be moot. Can you revert this change?

build is not autogenerated, but it is an ever present directory sitting at the top of the repo where we build the project. This is the canonical way to build a cmake project in Linux.

[pavanky]

Why is this necessary? I would prefer to use the standard add_compiler_export_flags? I see above in a comment that you say it's deprecated? Can you provide material that I can read?

@kknox On latest versions of CMake, you get the following warning when you use that function:

The add_compiler_export_flags function is obsolete. Use the
CXX_VISIBILITY_PRESET and VISIBILITY_INLINES_HIDDEN target properties
instead.

Now I am not sure when these options were added, but v3.0.2 seems to definitely have those options. If you are willing to change the minimum CMake requirement to 3.0, this can be fixed in the proper manner.

[pavanky]

You can see that the documentation mention that function is obsolete at the end of this page:
https://cmake.org/cmake/help/v3.0/module/GenerateExportHeader.html

[kknox]

This is the canonical way to build a cmake project in Linux.

I'm not advocating that you change your build layout, and I don't feel like you are doing anything wrong, but it's a specific requirement of your build environment and doesn't need to live upstream. I advocate its up to the user to decide where to compile, and this change encourages otherwise.

... this can be fixed in the proper manner. You can see that the documentation mention that function is obsolete at the end of this page.

Thank you for the heads up. I like your suggestion with regard to target properties, so could you refactor the code into:

set_target_properties(target PROPERTIES CXX_VISIBILITY_PRESET hidden)
set_target_properties(target PROPERTIES VISIBILITY_INLINES_HIDDEN ON )

My understanding is that this will do the right things on windows, and should also work for cmake 2.8.12. I think we would be good all around.

[9prady9]

@kknox All platform builds pass now.

[pavanky]

@9prady9 can you rebase the PR to have fewer commits ?

[pavanky]

This may not matter if @kknox wants to squash the commits while merging.

[kknox]

This looks great now; if you want to rebase, go ahead. If you are find with a merge/squash, i'll do that at the end of my business day.

Thank you for your contribution

[kknox]

Reviewed 4 of 4 files at r3.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

Comments from Reviewable

[pavanky]

@kknox pradeep is working on Indian time. I think it's fine if you merge + squash it :)