Closed tchaikov closed 10 months ago
@tchaikov See #1223; what problem was this PR solving for you? (What version of what compiler had both
@tchaikov See #1223; what problem was this PR solving for you? (What version of what compiler had both and string_view but not C++17?)
@jbeder hi Jesse, i was trying to address the comment of https://github.com/jbeder/yaml-cpp/pull/1148#issuecomment-1707632229 . but yeah, i was wrong following @0xFireWolf 's comment, as string_view
is a part of C++17, it should be safe to assume its existence as long as C++17. and if user has to use C++98, he/she won't even have the luxury of using string_view.
Oops, I completely forgot that feature-test macros are only available as of C++20. I apologize for my previous comment.
Now I see two possible options here:
std::string_view
exists._MSVC_LANG
seems to be initialized with a value w.r.t. the C++ standard. As such, you can first check whether the compiler is MSVC, and if so, you then check the value of _MSVC_LANG
, otherwise you check the value of __cplusplus
instead.I personally prefer the first option, because compilers that are still actively maintained all support C++17. However, if you care a lot about backward compatibility, then the second option seems to be a better solution.
Link to the description of _MSVC_LANG
.
@0xFireWolf i'd prefer using the platform neutral and standard compliant way. could you help me understand the advantage of checking _MSVC_LANG
instead of __cplusplus
?
@tchaikov I made two suggestions in your latest PR. Simply reverting your commit won't fix the issue I mentioned in https://github.com/jbeder/yaml-cpp/pull/1148#issuecomment-1707632229 on Windows. yaml-cpp
is also available on popular C++ package managers, such as vcpkg
and conan
, so it is not a wise choice to ask all developers who use this library on Windows to add the compiler option /Zc:__cplusplus
after they upgrade to 0.8.x.
@tchaikov I made two suggestions in your latest PR. Simply reverting your commit won't fix the issue I mentioned in #1148 (comment) on Windows.
yaml-cpp
is also available on popular C++ package managers, such asvcpkg
andconan
, so it is not a wise choice to ask all developers who use this library on Windows to add the compiler option/Zc:__cplusplus
after they upgrade to 0.8.x.
i don't claim that reverting the commit address your concern. i am trying to address the regression. i think, that is more important to the existing users of this library. __cplusplus
is defined if the C++ source file is compiled using a standard compliant compiler. the document you referenced also puts
__cplusplus Defined as an integer literal value when the translation unit is compiled as C++. Otherwise, undefined.
so it is not a wise choice to ask all developers who use this library on Windows to add the compiler option /Zc:__cplusplus after they upgrade to 0.8.x.
i am confused, if they don't pass /Zc:__cplusplus
, what they will be missing?
Let me clarify the issue from the beginning, and I hope it could clarify your confusion.
You first PR (https://github.com/jbeder/yaml-cpp/pull/1148) added conversion support for std::string_view
. Since yaml-cpp
still supports C++11 (as specified in CMakeLists.txt:L99
, you enable the support for std::string_view
only if developers have enabled C++17 or later. Your implementation works fine if developers use GCC or Clang, but it does not work if they use MSVC on Windows. The template specialization is disabled because __cplusplus
is initialized to be 199711L
regardless of which C++ standard developers choose if they don't add the compiler option /Zc:__cplusplus
. Please refer to Microsoft's documentation for details (look for the table that lists all possible values).
Now, there are multiple possible solutions to this issue.
/Zc:__cplusplus
to their project settings.std::string_view
is always available and thus get rid of the check of __cplusplus
.Each solution has different tradeoffs. In my humble opinion, the first solution is not wise, because the library author must explicitly state this requirement in the documentation, while developers must manually add that compiler option if they haven't done so. The second solution, while I love it, does not care about backward compatibility and thus abandons all pre-C++17 developers. The third solution, which I suggested in your latest PR (https://github.com/jbeder/yaml-cpp/pull/1225), seems to be the best one, because it fixes the issue on Windows without affecting *nix and Mac developers.
Now back to myself. The C++20 project I am working on needs a YAML parser and supports Linux, macOS and Windows. I recently upgraded yaml-cpp
to 0.8.0 through Conan package manager, and my CI pipeline failed because MSVC complained that it could not use std::string_view
as the coding key. That's how I identified the issue and left my comment in https://github.com/jbeder/yaml-cpp/pull/1148, but I completely forgot that feature tests are only available as of C++20. That's why this PR has introduced a regression for those who use older C++ standards, and I sincerely apologize for any inconvenience. I propose a better solution to the issue, because we need to accommodate all platforms supported by this library.
@jbeder Please feel free to chime in and make the final decision. Thank you for the amazing library and @tchaikov for your contributions.
For your convenience, I copied the description of _MSVC_LANG
from here.
_MSVC_LANG
: Defined as an integer literal that specifies the C++ language standard targeted by the compiler.
- It's set only in code compiled as C++.
- The macro is the integer literal value
201402L
by default, or when the /std:c++14 compiler option is specified.- The macro is set to
201703L
if the /std:c++17 compiler option is specified.- The macro is set to
202002L
if the /std:c++20 compiler option is specified.- It's set to a higher, unspecified value when the /std:c++latest option is specified.
- Otherwise, the macro is undefined.
The
_MSVC_LANG
macro and /std (Specify language standard version) compiler options are available beginning in Visual Studio 2015 Update 3.
The description of __cplusplus
copied from here.
The
/Zc:__cplusplus
compiler option enables the__cplusplus
preprocessor macro to report an updated value for recent C++ language standards support. By default, Visual Studio always returns the value199711L
for the__cplusplus
preprocessor macro.
So _MSVC_LANG
is always set properly with respect to the standard that developers choose, so the new check I suggested in #1225 should work for both GCC/Clang and MSVC.
if ((defined(_MSVC_LANG) && _MSVC_LANG >= 201703L) || __cplusplus >= 201703L)
//...
#endif
thank you @0xFireWolf . now i get your point better. i was under the impression that __cplusplus
should always be defined as long as a standard compliant compiler compiles a C++ source file. and i think MSVC has being doing great in the standard conforming perspective. i still think so after reading https://devblogs.microsoft.com/cppblog/msvc-now-correctly-reports-__cplusplus/ and https://gitlab.kitware.com/cmake/cmake/-/issues/18837 . both MSVC and CMake developers made the decision based on practical reasons.
it does not work if they use MSVC on Windows
so, to be more specific, the status is that the projects compiled using MSVC but not with /Zc:__cplusplus
cannot use the conversion helper.
my CI pipeline failed because MSVC complained that it could not use std::string_view as the coding key
i don't get this part though. probably the error is not specific enough for me to understand. because before you upgraded to yaml-cpp 0.8.0, you should already be using your own converter, right? so, IIUC, you updated your code to use the new convertor. and all of a sudden, the CI fails on MSVC. but it passes on platforms where the compiler defines __cplusplus
for us.
the discrepancy caused by the missing __cplusplus
is indeed frustrating. i am sorry for this. but i don't quite in favor of checking for a certain compiler if we already have __cplusplus
.
actually there is one more option: to pass /Zc:__cplusplus
on behalf of MSVC developers. but i don't dare to do so. i share the opinion of Brad in https://gitlab.kitware.com/cmake/cmake/-/issues/18837 , it should be guarded by an option.
well, personally i consider the check for _MSVC_LANG
a workaround. and i don't have strong preference in this regard. if we have to go that way, probably we can use something like
#if __cplusplus >= 201703L
#define YAML_DEFINE_HAS_STRING_VIEW 1
#elif defined(_MSVC_LANG) && _MSVC_LANG >= 201703L
#define YAML_DEFINE_HAS_STRING_VIEW 1
#else
#define YAML_DEFINE_HAS_STRING_VIEW 0
#endif
in convert.h
. as this header is already guarded by the #ifndef
and pragma once
guards, we don't need to check if YAML_DEFINE_HAS_STRING_VIEW
is defined or not here.
I was under the impression that __cplusplus should always be defined as long as a standard compliant compiler compiles a C++ source file
MSVC always defines the macro __cplusplus
when compiling C++ code. The problem is the value of __cplusplus
set by MSVC. If developers do not specify the option /Zc:__cplusplus
, __cplusplus
is always 199711L
. As a result, __cplusplus >= 201703L
is false
, and the template specialization is disabled.
so, to be more specific, the status is that the projects compiled using MSVC but not with /Zc:__cplusplus cannot use the conversion helper.
Correct.
because before you upgraded to yaml-cpp 0.8.0, you should already be using your own converter, right?
Yes, I manually specialized the template for std::string_view
when I was using 0.7.0. When I upgraded to 0.8.0, I read the release note and found that yaml-cpp
provides native support for std::string_view
, so I removed my code and committed my changes. GCC and Clang reported no compiler error, but MSVC complained that it could not find convert<std::string_view>
.
but it passes on platforms where the compiler defines __cplusplus for us.
This is wrong. __cplusplus
is always defined when GCC, Clang, and MSVC compile C++ code.
actually there is one more option: to pass /Zc:__cplusplus on behalf of MSVC developers. but i don't dare to do so.
I don't think this is a good idea.
#if __cplusplus >= 201703L #define YAML_DEFINE_HAS_STRING_VIEW 1 #elif defined(_MSVC_LANG) && _MSVC_LANG >= 201703L #define YAML_DEFINE_HAS_STRING_VIEW 1 #else #define YAML_DEFINE_HAS_STRING_VIEW 0 #endif
This seems better, especially if the library uses std::string_view
in multiple places.
okay. sorry for the confusion, i should have replaced "define" with "define the right".
in 35b44980, the conversion for
std::string_view
was added, but it enables it only if C++17 is used. but the availability ofstd::string_view
does not depends on C++17. in this change, we use the more specific language feature macro to check the availability ofstd::string_view
instead of checking for the C++ standard.Refs #1148 Signed-off-by: Kefu Chai tchaikov@gmail.com