How can I use std::string_view as the json_key to "operator []" ?

[jencoldeng]

//as follow, this will fail as the key_type pof map is std::string_view
std::map<string_view, bool> mv{{"111",true}, {"222", false}, {"333", true}};
nlohmann::json j4{mv};

//another: fail again, the std::string_view is the json_key
nlohmann::json j5;
std::string_view key = "my_key"sv;
j5[key] = 100;

[nlohmann]

The library does not support string views there. Object keys are std::string, so even if you could pass a string view, it would be first converted to a std::string for the lookup.

[jencoldeng]

but I can use string_view in map<std::string, bool>, like this:

std::map<std::string, bool, std::less<>> mp{{"111",true}, {"222", false}, {"333", true}}; //use std::less<>
std::string_view k = "111"sv;
const auto& it = mp.find(k);
cout<second<<endl;

as your json_object is a map type, why can't you do it similarly ?

[jencoldeng]

Maybe some part of your code I have not understand, : )

[nlohmann]

Maybe under the hood, std::map is ready to do the job, but the interfaces of the library mess it up. I need to check. Unfortunately, the library targets C++11, so I cannot do too much for std::string_view being a C++17 feature...

[jencoldeng]

What a pity : )

[nlohmann]

Transparent comparators seem to be a C++14 extension. :/

[nlohmann]

Any idea how to proceed?

[jencoldeng]

See my code, I add a operator[]:

 //by jencol
    template<typename keyT>
    reference operator[](keyT&& key)
    {
        // implicitly convert null to object
        if (is_null())
        {
            m_type = value_t::object;
            m_value = value_t::object;
            assert_invariant();
        }

        // at only works for objects
        if (JSON_LIKELY(is_object()))
        {
                        auto it = m_value.object->find(key);
                        if(it == m_value.object->end())
                        {
                                typename std::remove_reference<decltype(*(m_value.object))>::type::key_type _key{key};
                                return (*(m_value.object))[_key];
                        }
                        else
                        {
                                return it->second;
                        }
        }

        JSON_THROW(type_error::create(305, "cannot use operator[] with a string argument with " + std::string(type_name())));
    }

[jencoldeng]

See my code, I add a operator[]:

 //by jencol
    template<typename keyT>
    reference operator[](keyT&& key)
    {
        // implicitly convert null to object
        if (is_null())
        {
            m_type = value_t::object;
            m_value = value_t::object;
            assert_invariant();
        }

        // at only works for objects
        if (JSON_LIKELY(is_object()))
        {
                        auto it = m_value.object->find(key);
                        if(it == m_value.object->end())
                        {
                                typename std::remove_reference<decltype(*(m_value.object))>::type::key_type _key{key};
                                return (*(m_value.object))[_key];
                        }
                        else
                        {
                                return it->second;
                        }
        }

        JSON_THROW(type_error::create(305, "cannot use operator[] with a string argument with " + std::string(type_name())));
    }

Of course, it can be faster using std::map::emplace()

[nlohmann]

Wouldn't

reference operator[](std::string_view key)
{
    return operator[](key.data());
}

be already sufficient or is this too naive?

[jencoldeng]

It cannot be this.

when the key is string_view type, map.find(key); and map.find(key.data()); is different.

because a string_view object maybe points to a part of a continuous memory.

[nlohmann]

Looking at your code, you seem to convert the string view in whatever key type the map has? Right? Then it would be:

reference operator[](std::string_view key)
{
    return operator[](std::string(key));
}

[jencoldeng]

yes, exactly.

but my code above can work with cpp11/cpp14/cpp17, the template KeyT can be any type that can convert to key_type (such as std::string), not only for std::string_view. This can be determined at compile time.

[nlohmann]

Oh, I see. I still tried to figure out whether some "magic" is needed. In your example, I do not like the find call as operator[] should not perform bound checking. But the gist is clear.

[jencoldeng]

I alaways tryto make the code run faster.
so I only allocate memory only needed, such as make a std::string.
That's why I use find() before operator[], if find() found the key, then no need to generate std::string (allocate memory, copy data, deallocate memory...)

[nlohmann]

Transparent comparators seem to be a C++14 extension. :/

I just realized we already take care of this:

#if defined(JSON_HAS_CPP_14)
    // Use transparent comparator if possible, combined with perfect forwarding
    // on find() and count() calls prevents unnecessary string construction.
    using object_comparator_t = std::less<>;
#else
    using object_comparator_t = std::less<StringType>;
#endif

[jencoldeng]

Before cpp14, std::less<StringType> needs two StringType, so, in operator[] the KeyT should be convert to std::string explicitly before find().

After cpp14, it's ok to use std::less<>

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[jonesmz]

@nlohmann This is causing me a lot of problems.

I have string_view's all over my code (C++17) but am having to construct std::string objects just for the sake of calling nlohmann::json::operator[]. This is really harming performance, as well as being a pain to type out.

Can a compile time option be added to the library to support this in some manner?

[jencoldeng]

make nlohmann::json::operator[] to be a string_type template function

[jencoldeng]

something is similar in rapidjson, rapidjson::StringRef

[MikeGitb]

This is still not fixed - correct?

[psiha]

ping&upvote

[kentf]

I'm interested in being able to use std::string_view for both .at() and operator[].

A silver lining is that std::string will use SSO (short string optimization) when strings/keys are sufficiently short, avoiding the heap allocation penalty. Current wisdom from Stack Overflow says for 32-bit, the limit is about 10 chars, while for 64-bit it's 22 chars.

[MBalszun]

I'd also be very much interested in this. If no one else finds the time to pick this up, I might give it a try.

However, I'd first like to know if this lib will stay c++11 in the forseeable future or if there is a new major version on the horizon that will require c++14 or even 17. Implementing this in a purely optional and compatible manner may or may not be much more tricky than when you can just rely on std::string_view and transparent comparison being available.

[MBalszun]

@nlohmann: Are you seeing activity on closed issues?

[nlohmann]

I am running out of ideas here. I tried to bring the last working commit 15e4598 up to the develop branch in #3237, but this is also not working. As I obviously have no idea what I'm doing here, I will stop experimenting here now. If anyone can create a POC branch based on develop, I will happily take over and add tests and documentation.

[kentf]

@nlohmann; Thank you for the effort you're doing. I'll try to have a look this evening. It is branch string_view2, correct?

[nlohmann]

@nlohmann; Thank you for the effort you're doing. I'll try to have a look this evening. It is branch string_view2, correct?

Thanks, @kentf. string_view2 was created using

git checkout -b string_view2 5379b5d0910f5f712b80da09952c667e1ea21b29

to get back to the last working state of branch string_view. However, as there were a lot of changes to develop since then, I had to resolve a lot of merge conflicts. Assuming I did not even add more harm by resolving the conflicts, string_view2 should be a good place to start.

[kentf]

I've had a look, and I'm still looking. Currently I'm using MSVC 2022. By default, when configuring nlohmann/json with CMake, the default standard is C++14. I've added an option to CMakeLists.txt file to force either C++14, C++17 or C++20.

When using C++14 and C++17, they both fail to compile test-regression2. With C++20, there are no build errors.

It fails to compile on this line:

json/test/src/unit-regression2.cpp

Line 788 in 7254b1f

test3[json::json_pointer(p)] = json::object();

Will continue digging.

[TaeZStkyoht]

Is there a way to solve this issue? It really would be nice if someone implemented it.

[kentf]

Is there a way to solve this issue? It really would be nice if someone implemented it.

I know, right?

Most of the code for string_view is implemented, but there is a regression for MSVC when building in C++17. C++14 and C++20 are ok.

In this snippet, the problem is t1 and t4.

std::string p = "/root";
auto normal_ptr = json::json_pointer(p);
auto ordered_ptr = ordered_json::json_pointer(p);

json normal;
ordered_json ordered;

auto t1 = normal[ordered_ptr];
auto t2 = normal[normal_ptr];

auto t3 = ordered[ordered_ptr];
auto t4 = ordered[normal_ptr];

The compiler is not able to figure out the conversion between json_ordered and json.

[MBalszun]

I can think of a few ways to fix that (e.g. just handle foreign json pointer explicitly), but what I'd like to understand first is what changed between 14,17 and 20 that makes this not work for c++17 specifically.

[gregmarr]

@kentf On Jan 3 you said that 20 was okay, but 14 and 17 failed. Has that changed?

[gregmarr]

I'm wondering if json_pointer actually needs to be templated on the basic_json type, or if it can be changed so that just the functions that take a basic_json object can be templates. This looks like it would require some changes to the exception creation function to take a basic_json object or pointer instead of the template as there are functions that create an exception but don't take a basic_json object.

[MBalszun]

I'm wondering if json_pointer actually needs to be templated on the basic_json type, or if it can be changed so that just the functions that take a basic_json object can be templates.

By the looks of it, you are right, however. I wonder if it was ever the plan to use the same string type as in basic_json instead of std::string.

Regardless, my feeling is that such a change should not be part of this PR, because it would probably be a breaking change (e.g. in the unlikely case if someone overloaded a function for different json_pointer specializations) but on the other hand a meaningful improvement on its own.

[MBalszun]

Btw.: A different, but related change to json_pointer would be to change the implementation from std::vector<std::string> to std::string + std::vector<ReferenceTokenIndex>, such that you can convert a json_pointer to a std::string_view and of course construction of s json_pointer from a std::string_view might also be nice (although less important than basic_json's compatibility)

[kentf]

@kentf On Jan 3 you said that 20 was okay, but 14 and 17 failed. Has that changed?

It has sort of changed. I'm not sure what, but as of now C++14 and C++20 compiles fine. Only C++17 fails to build (in VC2022). It's the test project test-regression2_cpp17, which fails to build when mixing json::json_pointer and ordered_json::json_pointer.

With commit 7254b1f6df6b80ee604b86364be1cf553e11f322 as baseline, the following patch is enough to create a C++20 compilation of the unit test project. Otherwise, if C++20 is not needed, this patch is not necessary to reproduce the build error.

diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
index 4c741f274..bd1b24f6c 100644
--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -57,14 +57,19 @@ foreach(feature ${CMAKE_CXX_COMPILE_FEATURES})
     if (${feature} STREQUAL cxx_std_17)
         set(compiler_supports_cpp_17 TRUE)
     endif()
+    if (${feature} STREQUAL cxx_std_20)
+        set(compiler_supports_cpp_20 TRUE)
+    endif()
 endforeach()
 # Clang only supports C++17 starting from Clang 5.0
 if ("${CMAKE_CXX_COMPILER_ID}" MATCHES "Clang" AND CMAKE_CXX_COMPILER_VERSION VERSION_LESS 5.0)
     unset(compiler_supports_cpp_17)
+    unset(compiler_supports_cpp_20)
 endif()
 # MSVC 2015 (14.0) does not support C++17
 if ("${CMAKE_CXX_COMPILER_ID}" MATCHES "MSVC" AND CMAKE_CXX_COMPILER_VERSION VERSION_LESS 19.1)
     unset(compiler_supports_cpp_17)
+    unset(compiler_supports_cpp_20)
 endif()
 
 file(GLOB files src/unit-*.cpp)
@@ -119,6 +124,43 @@ foreach(file ${files})
         endif()
     endif()
 
+    # add a copy with C++20 compilation
+    if (compiler_supports_cpp_20)
+        file(READ ${file} FILE_CONTENT)
+
+        string(FIND "${FILE_CONTENT}" "JSON_HAS_CPP_20" CPP_20_FOUND)
+        if(NOT ${CPP_20_FOUND} EQUAL -1)
+            add_executable(${testcase}_cpp20 $<TARGET_OBJECTS:doctest_main> ${file})
+            target_compile_definitions(${testcase}_cpp20 PRIVATE DOCTEST_CONFIG_SUPER_FAST_ASSERTS)
+            target_compile_options(${testcase}_cpp20 PRIVATE
+                $<$<CXX_COMPILER_ID:MSVC>:/EHsc;$<$<CONFIG:Release>:/Od>>
+                $<$<NOT:$<CXX_COMPILER_ID:MSVC>>:-Wno-deprecated;-Wno-float-equal>
+                $<$<CXX_COMPILER_ID:GNU>:-Wno-deprecated-declarations>
+            )
+            target_include_directories(${testcase}_cpp20 PRIVATE ${CMAKE_BINARY_DIR}/include thirdparty/doctest thirdparty/fifo_map)
+            target_link_libraries(${testcase}_cpp20 PRIVATE ${NLOHMANN_JSON_TARGET_NAME})
+            target_compile_features(${testcase}_cpp20 PRIVATE cxx_std_20)
+
+            if (CMAKE_CXX_COMPILER_ID STREQUAL "GNU" AND CMAKE_CXX_COMPILER_VERSION VERSION_GREATER_EQUAL 8.0 AND CMAKE_CXX_COMPILER_VERSION VERSION_LESS 9.0 AND NOT MINGW)
+                # fix for https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90050
+                target_link_libraries(${testcase}_cpp20 PRIVATE stdc++fs)
+            endif()
+
+            if (JSON_FastTests)
+                add_test(NAME "${testcase}_cpp20"
+                    COMMAND ${testcase}_cpp20 ${DOCTEST_TEST_FILTER}
+                    WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}
+                )
+            else()
+                add_test(NAME "${testcase}_cpp20"
+                    COMMAND ${testcase}_cpp20 ${DOCTEST_TEST_FILTER} --no-skip
+                    WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}
+                )
+            endif()
+            set_tests_properties("${testcase}_cpp20" PROPERTIES LABELS "all" FIXTURES_REQUIRED TEST_DATA)
+        endif()
+    endif()
+
     if (JSON_FastTests)
         add_test(NAME "${testcase}"
             COMMAND ${testcase} ${DOCTEST_TEST_FILTER}
diff --git a/test/src/unit-regression2.cpp b/test/src/unit-regression2.cpp
index afbdf3dbe..4b6ad78f0 100644
--- a/test/src/unit-regression2.cpp
+++ b/test/src/unit-regression2.cpp
@@ -100,6 +100,10 @@ using ordered_json = nlohmann::ordered_json;
     #endif
 #endif
 
+#ifdef JSON_HAS_CPP_20
+// Trigger C++20 compilation by having JSON_HAS_CPP_20 present in the file
+#endif
+
 #ifndef JSON_HAS_EXPERIMENTAL_FILESYSTEM
     #define JSON_HAS_EXPERIMENTAL_FILESYSTEM 0
 #endif

The (C++17) compiler chokes with this error:

1>R:\Projects\json\test\src\unit-regression2.cpp(792,36): error C2679: binary '[': no operator found which takes a right-hand operand of type 'nlohmann::json_pointer<nlohmann::basic_json<std::map,std::vector,std::string,bool,int64_t,uint64_t,double,std::allocator,nlohmann::adl_serializer,std::vector<uint8_t,std::allocator<uint8_t>>>>' (or there is no acceptable conversion)
1>R:\Projects\json\include\nlohmann/json.hpp(4224,21): message : could be 'const nlohmann::basic_json<nlohmann::ordered_map,std::vector,std::string,bool,int64_t,uint64_t,double,std::allocator,nlohmann::adl_serializer,std::vector<uint8_t,std::allocator<uint8_t>>> &nlohmann::basic_json<nlohmann::ordered_map,std::vector,std::string,bool,int64_t,uint64_t,double,std::allocator,nlohmann::adl_serializer,std::vector<uint8_t,std::allocator<uint8_t>>>::operator [](const nlohmann::json_pointer<nlohmann::basic_json<nlohmann::ordered_map,std::vector,std::string,bool,int64_t,uint64_t,double,std::allocator,nlohmann::adl_serializer,std::vector<uint8_t,std::allocator<uint8_t>>>> &) const'
1>R:\Projects\json\include\nlohmann/json.hpp(4217,15): message : or       'nlohmann::basic_json<nlohmann::ordered_map,std::vector,std::string,bool,int64_t,uint64_t,double,std::allocator,nlohmann::adl_serializer,std::vector<uint8_t,std::allocator<uint8_t>>> &nlohmann::basic_json<nlohmann::ordered_map,std::vector,std::string,bool,int64_t,uint64_t,double,std::allocator,nlohmann::adl_serializer,std::vector<uint8_t,std::allocator<uint8_t>>>::operator [](const nlohmann::json_pointer<nlohmann::basic_json<nlohmann::ordered_map,std::vector,std::string,bool,int64_t,uint64_t,double,std::allocator,nlohmann::adl_serializer,std::vector<uint8_t,std::allocator<uint8_t>>>> &)'
1>R:\Projects\json\include\nlohmann/json.hpp(2062,21): message : or       'const nlohmann::basic_json<nlohmann::ordered_map,std::vector,std::string,bool,int64_t,uint64_t,double,std::allocator,nlohmann::adl_serializer,std::vector<uint8_t,std::allocator<uint8_t>>> &nlohmann::basic_json<nlohmann::ordered_map,std::vector,std::string,bool,int64_t,uint64_t,double,std::allocator,nlohmann::adl_serializer,std::vector<uint8_t,std::allocator<uint8_t>>>::operator [](nlohmann::basic_json<nlohmann::ordered_map,std::vector,std::string,bool,int64_t,uint64_t,double,std::allocator,nlohmann::adl_serializer,std::vector<uint8_t,std::allocator<uint8_t>>>::size_type) const'
1>R:\Projects\json\include\nlohmann/json.hpp(2016,15): message : or       'nlohmann::basic_json<nlohmann::ordered_map,std::vector,std::string,bool,int64_t,uint64_t,double,std::allocator,nlohmann::adl_serializer,std::vector<uint8_t,std::allocator<uint8_t>>> &nlohmann::basic_json<nlohmann::ordered_map,std::vector,std::string,bool,int64_t,uint64_t,double,std::allocator,nlohmann::adl_serializer,std::vector<uint8_t,std::allocator<uint8_t>>>::operator [](nlohmann::basic_json<nlohmann::ordered_map,std::vector,std::string,bool,int64_t,uint64_t,double,std::allocator,nlohmann::adl_serializer,std::vector<uint8_t,std::allocator<uint8_t>>>::size_type)'
1>R:\Projects\json\test\src\unit-regression2.cpp(792,36): message : while trying to match the argument list '(ordered_json, nlohmann::json_pointer<nlohmann::basic_json<std::map,std::vector,std::string,bool,int64_t,uint64_t,double,std::allocator,nlohmann::adl_serializer,std::vector<uint8_t,std::allocator<uint8_t>>>>)'

I'm not too well versed on the internals of this magnificent library, so I'm not qualified to give inputs on what should be done next. But it seems like there is an issues of a missing implicit conversion from std::map and to either nlohmann::map/nlohmann::ordered_map?

[MBalszun]

But it seems like there is an issues of a missing implicit conversion from std::map and to either nlohmann::map/nlohmann::ordered_map?

Superficially, the problem seems to be that the foreign_json pointer doesn't pass the detail::is_usable_as_key_type<basic_json_t, KeyT>::value constraint and thus the templetized function isn't considerd for overload resolution. That is why you only see the overloads for size_type and the matiching json_pointer in the above error message.

Now, one way to handle that (I assume, I didn't test it) would be to explicitly handle a json_ponter, that is not the same as self_type::json_pointer in the detail::is_usable_as_key_type<basic_json_t, KeyT>::value constraint.

However, out of professional pride, I would prefer if I could understand the root cause and offer a more "elegant" solution than that. But that should not stop anyone of you for applying above suggestion (assuming it actually works).

[falbrechtskirchinger]

So, I spent all of yesterday on this issue without reading this thread first (regretfully, *sigh*). Long story short, I'm very confused by what the correct behavior is supposed to be.

Here are some of the options:

a) Foreign json_pointers are expected to implicitly convert to string and call the matching operator[]. This is how it's codified in the regression test and what I based my solution on. However, it seems semantically incorrect. Is this wanted for backwards compatibility?

b) Foreign json_pointers behave the same as native json_pointers. This one should be easy to implement by templating some member functions of json_pointer.

c) Foreign json_pointers should fail to compile.

I assume, removing the BasicJsonType class template parameter from json_pointer is not an option? Even so it seems doable after a first glance. Exceptions may need an overload without const BasicJsonType &context parameter as json_pointer is full of JSON_THROW(..., BasicJsonType()). Might as well omit context. The latter could be done anyway and might pave the way for a change later.

Personally, option B seems like the way to go.

The commit history on the string_view2 branch is quite difficult to read. For my own benefit, I'd like to clean it up before I start working on any of the options.
Any recommendations on how I should handle this? I'd like to create a string_view3 branch, but also preserve credit of previous authors, reviewers, etc. by keeping notes and adding them to the final commit message before it's ready to be merged. Is that acceptable?

I've some additional ideas surrounding object_comparator_t, ordered_map, and is_usable_as_key_type, but those need more time to develop. (I dislike that ordered_map is passed object_comparator_t without actually using it and is_usable_as_key_type is based on comparability determined by object_comparator_t.)

@nlohmann could you please chime in?

[gregmarr]

I assume, removing the BasicJsonType class template parameter from json_pointer is not an option? Even so it seems doable after a first glance. Exceptions may need an overload without const BasicJsonType &context parameter as json_pointer is full of JSON_THROW(..., BasicJsonType()). Might as well omit context. The latter could be done anyway and might pave the way for a change later.

That is similar to my suggestion here:

#1529 (comment)

[falbrechtskirchinger]

I assume, removing the BasicJsonType class template parameter from json_pointer is not an option? Even so it seems doable after a first glance. Exceptions may need an overload without const BasicJsonType &context parameter as json_pointer is full of JSON_THROW(..., BasicJsonType()). Might as well omit context. The latter could be done anyway and might pave the way for a change later.

That is similar to my suggestion here:

#1529 (comment)

Indeed, but no one else weighed in. What is this projects policy on API stability? Would that even be considered? Maybe for 4.0?
For what it's worth I'm not in favor of such a change for 3.x.

Yesterday I decoupled all relevant member functions from the class template parameter via a function template parameter (exceptions still need an overload as described earlier, before the template parameter could be removed). CI on my PoC string_view3 branch is all green.

I just need to know if this snippet from the regression test shows the intended and desired behavior:

// mixed type - the JSON Pointer is implicitly converted into a string "/root"
ordered_json test3;
test3[json::json_pointer(p)] = json::object();
CHECK(test3.dump() == "{\"/root\":{}}");

I've modified this to produce and expect an object with key "root" (no leading slash). Makes more sense to me than the foreign json_pointer implicitly converting to string "/root" (leading slash).