Skip to content
This repository was archived by the owner on Mar 22, 2023. It is now read-only.

Fix Clang detection#1102

Merged
lukaszstolarczuk merged 1 commit intopmem:masterfrom
michalbiesek:fix-clang-detection
Jun 25, 2021
Merged

Fix Clang detection#1102
lukaszstolarczuk merged 1 commit intopmem:masterfrom
michalbiesek:fix-clang-detection

Conversation

@michalbiesek
Copy link
Copy Markdown

@michalbiesek michalbiesek commented Jun 25, 2021


This change is Reviewable

Copy link
Copy Markdown
Contributor

@igchor igchor left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @michalbiesek)

@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 25, 2021

Codecov Report

Merging #1102 (1e6a675) into master (1db710e) will decrease coverage by 0.07%.
The diff coverage is n/a.

❗ Current head 1e6a675 differs from pull request most recent head 7e916ad. Consider uploading reports for the commit 7e916ad to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1102      +/-   ##
==========================================
- Coverage   94.39%   94.31%   -0.08%     
==========================================
  Files          51       51              
  Lines        5193     5193              
==========================================
- Hits         4902     4898       -4     
- Misses        291      295       +4     
Flag Coverage Δ
tests_clang_debug_cpp17 93.85% <ø> (-0.03%) ⬇️
tests_gcc_debug 91.82% <ø> (-0.11%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...libpmemobj++/detail/enumerable_thread_specific.hpp 98.91% <0.00%> (-1.09%) ⬇️
include/libpmemobj++/transaction.hpp 79.83% <0.00%> (-0.81%) ⬇️
...ude/libpmemobj++/container/concurrent_hash_map.hpp 94.20% <0.00%> (-0.32%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1db710e...7e916ad. Read the comment docs.

Copy link
Copy Markdown
Member

@lukaszstolarczuk lukaszstolarczuk left a comment

Choose a reason for hiding this comment

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

this looks good! thanks for this change

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @michalbiesek)


CMakeLists.txt, line 157 at r1 (raw file):

endif()

# Find Clang

with this approach, we don't even need this status printing, I believe.

The current compiler is checked at the beginning of the CMake execution, e.g.:

-- The CXX compiler identification is Clang 9.0.1
-- Check for working CXX compiler: /usr/bin/clang++-9
-- Check for working CXX compiler: /usr/bin/clang++-9 -- works

tests/ctest_helpers.cmake, line 142 at r1 (raw file):

function(build_test_atomic name)
	build_test(${name} ${ARGN})
	if (CMAKE_CXX_COMPILER_ID STREQUAL "Clang")

what about CMAKE_C_COMPILER_ID ?

we have some check in/tests/CMakeLists.txt file for GNU, and we check both variables...


tests/ctest_helpers.cmake, line 142 at r1 (raw file):

function(build_test_atomic name)
	build_test(${name} ${ARGN})
	if (CMAKE_CXX_COMPILER_ID STREQUAL "Clang")

how about MATCHES instead of STREQUAL...?

@michalbiesek michalbiesek force-pushed the fix-clang-detection branch from 1e6a675 to fa5db78 Compare June 25, 2021 12:56
Copy link
Copy Markdown
Author

@michalbiesek michalbiesek left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @igchor and @lukaszstolarczuk)


CMakeLists.txt, line 157 at r1 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

with this approach, we don't even need this status printing, I believe.

The current compiler is checked at the beginning of the CMake execution, e.g.:

-- The CXX compiler identification is Clang 9.0.1
-- Check for working CXX compiler: /usr/bin/clang++-9
-- Check for working CXX compiler: /usr/bin/clang++-9 -- works

Done (removed)


tests/ctest_helpers.cmake, line 142 at r1 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

how about MATCHES instead of STREQUAL...?

Done.


tests/ctest_helpers.cmake, line 142 at r1 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

what about CMAKE_C_COMPILER_ID ?

we have some check in/tests/CMakeLists.txt file for GNU, and we check both variables...

Done - out of curiosity can you give me the use case where you set the "C compiler" and for the CPP as a different one e.g. Clang for C and GCC for C++

- use CMAKE_CXX_COMPILER_VERSION/CMAKE_C[XX]_COMPILER_ID for
  setting compiler conditions
- fixes pmem#1090
@michalbiesek michalbiesek force-pushed the fix-clang-detection branch from fa5db78 to 7e916ad Compare June 25, 2021 13:07
Copy link
Copy Markdown
Member

@lukaszstolarczuk lukaszstolarczuk left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 2 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @michalbiesek)


tests/ctest_helpers.cmake, line 142 at r1 (raw file):

Previously, michalbiesek wrote…

Done - out of curiosity can you give me the use case where you set the "C compiler" and for the CPP as a different one e.g. Clang for C and GCC for C++

yeah, there's probably none, that's why it was more of an open question (rather than a blocking issue). Thx for updating

@lukaszstolarczuk lukaszstolarczuk merged commit 5709a7e into pmem:master Jun 25, 2021
@michalbiesek michalbiesek deleted the fix-clang-detection branch June 25, 2021 16:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Clang detection does not work if there is no clang (but e.g. clang-9)

3 participants