jbeder / yaml-cpp

A YAML parser and emitter in C++
MIT License
4.92k stars 1.78k forks source link

node/convert: support conversion for std::string_view #1148

Closed tchaikov closed 11 months ago

tchaikov commented 1 year ago

as the key is not always nul terminated, it'd be handy to use a std::string_view when a key is expected.

before this change, we would have FTBFS if the key is a string_view when looking up a node in a map:

/usr/include/yaml-cpp/node/detail/impl.h: In instantiation of ‘bool YAML::detail::node::equals(const T&, YAML::detail::shared_memory_holder) [with T = std::basic_string_view<char>; YAML::detail::shared_memory_holder = std::shared_ptr<YAML::detail::memory_holder>]’:
/usr/include/yaml-cpp/node/detail/impl.h:122:26:   required from ‘YAML::detail::node* YAML::detail::node_data::get(const Key&, YAML::detail::shared_memory_holder) const [with Key = std::basic_string_view<char>; YAML::detail::shared_memory_holder = std::shared_ptr<YAML::detail::memory_holder>]’
/usr/include/yaml-cpp/node/detail/node_ref.h:64:55:   required from ‘YAML::detail::node* YAML::detail::node_ref::get(const Key&, YAML::detail::shared_memory_holder) const [with Key = std::basic_string_view<char>; YAML::detail::shared_memory_holder = std::shared_ptr<YAML::detail::memory_holder>]’
/usr/include/yaml-cpp/node/detail/node.h:125:53:   required from ‘YAML::detail::node* YAML::detail::node::get(const Key&, YAML::detail::shared_memory_holder) const [with Key = std::basic_string_view<char>; YAML::detail::shared_memory_holder = std::shared_ptr<YAML::detail::memory_holder>]’
/usr/include/yaml-cpp/node/impl.h:392:71:   required from ‘const YAML::Node YAML::Node::operator[](const Key&) const [with Key = std::basic_string_view<char>]’
../../src/config/base_config.cc:20:23:   required from here
/usr/include/yaml-cpp/node/detail/impl.h:93:25: error: incomplete type ‘YAML::convert<std::basic_string_view<char> >’ used in nested name specifier
   93 |   if (convert<T>::decode(Node(*this, pMemory), lhs)) {
      |       ~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~

after this change, a full specialization for convert<std::string_view is added so that we can lookup a node using key of std::string_view, like

string_view key = "yet_another_key";
auto value = node[key];

tested with

$ cmake -B build -D CMAKE_CXX_STANDARD=20 -D YAML_CPP_BUILD_TESTS=ON -G Ninja
$ cmake --build build yaml-cpp-tests
$ ctest --test-dir build --output-on-failure

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

tchaikov commented 1 year ago

@jbeder hi Jesse, could you please help review this change at your convenience?

tchaikov commented 1 year ago

@jbeder hello Jesse, ping for reviews.

jbeder commented 1 year ago

Can you sync to head so it can run the tests? Otherwise LGTM.

tchaikov commented 1 year ago

@jbeder sure. thank you for your review! rebased and repushed.

tchaikov commented 1 year ago
[0 / 1] [Prepa] BazelWorkspaceStatusAction stable-status.txt
ERROR: D:/a/yaml-cpp/yaml-cpp/BUILD.bazel:14:11: Compiling src/binary.cpp failed: (Exit 1): vc_installation_error_x64.bat failed: error executing command (from target //:yaml-cpp) external\local_config_cc\vc_installation_error_x64.bat /nologo /DCOMPILER_MSVC /DNOMINMAX /D_WIN32_WINNT=0x0601 /D_CRT_SECURE_NO_DEPRECATE /D_CRT_SECURE_NO_WARNINGS /bigobj /Zm500 /EHsc /wd4351 /wd4291 ... (remaining 20 arguments skipped)

The target you are compiling requires Visual C++ build tools. 
Bazel couldn't find a valid Visual C++ build tools installation on your machine. 

Visual C++ build tools seems to be installed at C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC 
But Bazel can't find the following tools: 
    VCVARSALL.BAT, cl.exe, link.exe, lib.exe, ml64.exe 
for x64 target architecture 

Please check your installation following https://bazel.build/docs/windows#using 

my guess is that this failure is an occurrence of https://github.com/bazelbuild/bazel/issues/18592 .

jbeder commented 1 year ago

Yeah, I think you're right. Bad timing :)

It looks like they're working on a fix, so once they release it, we can rerun and then I'll merge this PR. (I hesitate to merge it with a broken test even if it's almost certainly unrelated.)

tchaikov commented 1 year ago

sure. let's keep an eye on the update on this bazel issue!

tchaikov commented 11 months ago

@jbeder hi Jesse, it seems that https://github.com/bazelbuild/bazel/issues/18592 has been fixed. could you please give a green light to the workflow, so we can verify the change?

0xFireWolf commented 10 months ago

Thanks for your work, but I found an issue after upgrading to 0.8.0.

MSVC initializes __cplusplus with a value of 199711L, if users don't specify the option Zc:__cplusplus explicitly. As a result, your template specialization is disabled by default on Windows. I wonder whether it would be better to use the feature-test macro __cpp_lib_string_view` instead. What's your opinions? @tchaikov @jbeder

#if __cpp_lib_string_view >= 201606L
// ...
#endif
tchaikov commented 10 months ago
__cpp_lib_string_view

@0xFireWolf indeed. but instead of assuming the existence of __cpp_lib_string_view, i'd prefer checking its existence instead, see #1222

tchaikov commented 10 months ago

Thanks for your work, but I found an issue after upgrading to 0.8.0.

MSVC initializes __cplusplus with a value of 199711L, if users don't specify the option Zc:__cplusplus explicitly. As a result, your template specialization is disabled by default on Windows. I wonder whether it would be better to use the feature-test macro __cpp_lib_string_view` instead. What's your opinions? @tchaikov @jbeder

#if __cpp_lib_string_view >= 201606L
// ...
#endif

@0xFireWolf yaml-cpp 0.6.0 and up requires C++11 and up. i don't think we are able to use C++98 with the master HEAD. how do you test yaml-cpp in your project?

0xFireWolf commented 10 months ago

How do you test yaml-cpp in your project?

@tchaikov My project uses C++20 and is compiled by GCC 10+, Clang 13+, and VS2019+.

tchaikov commented 10 months ago

@0xFireWolf so why do you care about C++standard of 199711L?

0xFireWolf commented 10 months ago

Shall we keep the conversation in one place?

tchaikov commented 10 months ago

yeah. ahh, but kind of late. agreed, we should have used an issue for the discussion. but i am fine either place: here or https://github.com/jbeder/yaml-cpp/pull/1222 .