MSVC C++20 Support #126
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| /* | ||
| Copyright 2024 Adobe | ||
| Distributed under the Boost Software License, Version 1.0. | ||
| (See accompanying file LICENSE_1_0.txt or copy at http://www.boost.org/LICENSE_1_0.txt) | ||
| */ | ||
| /**************************************************************************************************/ | ||
|
|
||
| #ifndef ADOBE_ALGORITHM_APPEND_HPP | ||
| #define ADOBE_ALGORITHM_APPEND_HPP | ||
|
|
||
| /**************************************************************************************************/ | ||
|
|
||
| namespace adobe { | ||
|
|
||
| /**************************************************************************************************/ | ||
|
|
||
| /// Insert the range `r` at the end of the sequence container `x`. | ||
| /// Returns an iterator pointing to the first element inserted, or `x.end()` if `r` is empty. | ||
| template <class T, // T models SequenceContainer | ||
| class R> // R models Range | ||
| inline auto append(T& x, const R& r) { | ||
| return x.insert(std::end(x), std::begin(r), std::end(r)); | ||
| } | ||
|
|
||
| /**************************************************************************************************/ | ||
|
|
||
| } // namespace adobe | ||
|
|
||
| /**************************************************************************************************/ | ||
|
|
||
| #endif | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,9 +7,6 @@ function(asl_test) | |
| add_executable (${asl_test_NAME} ${asl_test_SOURCES}) | ||
|
|
||
| if ("${CMAKE_CXX_COMPILER_ID}" STREQUAL "MSVC") | ||
| target_compile_options (${asl_test_NAME} PRIVATE "/W4") | ||
| target_compile_options (${asl_test_NAME} PRIVATE "/WX") | ||
| else() | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,9 +2,6 @@ add_executable (adam_smoke_test adam_smoke_test.cpp) | |
| target_link_libraries(adam_smoke_test PRIVATE asl) | ||
|
|
||
| if ("${CMAKE_CXX_COMPILER_ID}" STREQUAL "MSVC") | ||
| target_compile_options (adam_smoke_test PRIVATE "/W4") | ||
| target_compile_options (adam_smoke_test PRIVATE "/WX") | ||
| endif() | ||
|
There was a problem hiding this comment. @dabrahams @camio - There must be a better way to manage compiler settings. There was a problem hiding this comment. This is how I think about it: For portable libraries, the consumer of the library determines the compiler+flags combination for building the library. When the library writer dictates flags, the library gets tied to a snapshot of today's compilers and options making it unportable. The library writer can, however, require certain compiler capabilities. The requirement underlying the desire for if(WIN32)
check_cxx_symbol_exists(min WinDef.h ASL_WINDEF_DEFINES_MIN_MAX)
if(ASL_WINDEF_DEFINES_MIN_MAX)
message(FATAL_ERROR "WinDef.h defining `min` and `max` is unsupported by this library. If using MSVC, adding -DNOMINMAX to your compiler flags will address this issue.")
endif()
endif()The same goes for the other two options you want to require. The idea is to check for the functionality you require using something like The last two options that you want for a single file are only useful for developers of this library, but problematic for users and integrators (especially when those flags prevent this library from working with a new compiler version). If applying these flags to the entire repository isn't feasible, I'd suggest adding a CMake option (defaulting to FALSE) for enabling these flags. Then your code would look something like this: if (ASL_FORCE_WARNINGS AND "${CMAKE_CXX_COMPILER_ID}" STREQUAL "MSVC")
target_compile_options (adam_smoke_test PRIVATE "/W4")
target_compile_options (adam_smoke_test PRIVATE "/WX")
endif()The next question to ask is how are users going to know which flags they need to pass to the various compilers they use. This is what presets are for. With the right preset a user should be able to use There was a problem hiding this comment. If by
You actually mean you want the library to be built with the first 3 options and its clients to have the 2nd 2 options imposed on them automatically due to the fact that they link the library, then you want target_compile_options(the_library PRIVATE "/Zc:__cplusplus" PUBLIC "/W4" "/WX")The last two become usage requirements as well as requirements for building the library. To make them pure usage requirements, you'd s/ @camio's statement that:
Could be interpreted two ways: 1, you should try to use higher-level constructs than the flags for specific compilers whenever possible, or 2, you should never employ usage requirements. I agree with the first but would disagree with the second. I'm not sure what these flags do or whether higher-level constructs exist for them, though. There was a problem hiding this comment. I updated it to use PRIVATE/PUBLIC (more) appropriately. |
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Accepted.