node/convert: relax the check for string_view

[tchaikov]

in 35b4498, the conversion for std::string_view was added, but it enables it only if C++17 is used. but the availability of std::string_view does not depends on C++17. in this change, we use the more specific language feature macro to check the availability of std::string_view instead of checking for the C++ standard.

Refs #1148
Signed-off-by: Kefu Chai tchaikov@gmail.com

[jbeder]

@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?)

[tchaikov]

@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 #1148 (comment) . 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.

[0xFireWolf]

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:

• Raise the minimum C++ standard to C++17 as of 0.8.1, so you can assume that std::string_view exists.
• Keep the standard unchanged and enable the specialization only if users enable C++17. According to Microsoft's documentation, the MSVC-specific macro _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.

[0xFireWolf]

Link to the description of _MSVC_LANG.

[tchaikov]

@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 ?

[0xFireWolf]

@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 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]

@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 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.

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?

[0xFireWolf]

Let me clarify the issue from the beginning, and I hope it could clarify your confusion.

You first PR (#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.

1. Ask all developers who use this library on Windows to add the compiler option /Zc:__cplusplus to their project settings.
2. Raise the minimum C++ standard to C++17, so you can safely assume that std::string_view is always available and thus get rid of the check of __cplusplus.
3. Implement a more robust check to accommodate developers who use this library on Windows.

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 (#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 #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.

[0xFireWolf]

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 value 199711L 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

[tchaikov]

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.

[0xFireWolf]

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.

[tchaikov]

okay. sorry for the confusion, i should have replaced "define" with "define the right".