Clangtastic Voyage #20392
Clangtastic Voyage #20392
Conversation
Using an explicit {} initializer. This unbreaks Clang.
Signed-off-by: Adam C. Emerson <aemerson@redhat.com>
|
But why? Why is clang unhappy with the existing state of affairs, and why does this explicit empty initializer make it better? |
|
@gregsfortytwo As of 7e2f59a , inode_t is no longer a class, it's a template with a defaulted argument. Pre-C++17, you'd have to write inode_t<> everywhere you were writing inode_t as a default. The class deduction rules now let you avoid that, it can assume a defaulted template argument from the default constructor. HOWEVER, it looks like in this case Clang 5 is /slightly/ wrong, and it requires you specify an empty initializer to get the default constructor explicitly, whereas g++ correctly uses that with no initializer specified at all. I think Clang 6 fixes it, but it's not stable yet. |
CMakeLists.txt
Outdated
| @@ -594,6 +594,30 @@ include_directories(SYSTEM ${PROJECT_BINARY_DIR}/include) | |||
|
|
|||
| find_package(Threads REQUIRED) | |||
|
|
|||
| if(CMAKE_CXX_COMPILER_ID STREQUAL Clang) | |||
There was a problem hiding this comment.
@adamemerson thanks for getting this right. i am just curious why you choosed to check the output of c++ compiler instead of using _LIBCPP_VERSION and __GLIBCXX__ in the librarycheck.cc file?
There was a problem hiding this comment.
also, if we use the combination of gcc and libc++, shall we link against c++experimental as well?
There was a problem hiding this comment.
@tchaikov I think no?
There was a problem hiding this comment.
@mattbenjamin why? as i don't think stdc++fs should be linked if libc++ is used.
CMakeLists.txt
Outdated
| elseif(HUGE_COMPILE_OUTPUT MATCHES "\"-lc\\+\\+\"") | ||
| set(CLANG_CXXLIB "libc++") | ||
| message(STATUS "We are using libc++.") | ||
| elseif(HUGE_COMPILE_OUTPUT MATCHES "\"-lc\\+\\+\"") |
There was a problem hiding this comment.
this branch looks quite like the previous one.
CMakeLists.txt
Outdated
| ) | ||
|
|
||
| if(HUGE_COMPILE_OUTPUT MATCHES "\"-lstdc\\+\\+\"") | ||
| set(CLANG_CXXLIB "libstdc++") |
There was a problem hiding this comment.
if the answer of the previous question evaluates to "true". the variable name is sort of misleading. as it has clang in it. how about CXX_STDLIB?
src/test/CMakeLists.txt
Outdated
| @@ -508,10 +508,10 @@ add_executable(ceph_test_admin_socket_output | |||
| ) | |||
| target_link_libraries(ceph_test_admin_socket_output | |||
| ceph-common) | |||
| if(CMAKE_CXX_COMPILER_ID STREQUAL GNU) | |||
| if(CMAKE_CXX_COMPILER_ID STREQUAL GNU OR CLANG_CXXLIB STREQUAL "libstdc++") | |||
There was a problem hiding this comment.
thanks. but ... i think the library to be linked just depends on the C++ library we are using, so we probably we need to put
if(CLANG_CXXLIB STREQUAL "libstdc++")
# ...
elseif(CLANG_CXXLIB STREQUAL "libc++")
# ...
endif()instead. what do you think?
|
@gregsfortytwo because builds w/the clang-5 that ships w/F27 stopped working :) |
Test whether we're building with libstdc++ or libc++. Use stdc++filesystem if the former and c++experimental if the later. Signed-off-by: Adam C. Emerson <aemerson@redhat.com>
adamemerson
force-pushed
the
wip-clangtastic-voyage
branch
from
75600bf
to
ba5ce15
Compare
|
@tchaikov How's this? |
|
@tchaikov sorry, I probably misunderstood you; I thought I was joking about linking w/-lc++experimental |
|
Jenkins, retest this please. |
Fix Clang builds.
(Including Clang builds under Linux using libstdc++.)