Add transparent comparator and perfect forwarding support to find() and count() #795

jseward · 2017-10-21T22:26:04Z

This avoids unnecessary string copies and allocation on often used find() if std::string_view is supported.

NOTE: currently only implemented for MSVS 2017, should be easily enabled for other compilers as well but don't have them available.

This avoids unnecessary string copies on often used find().

Copied from solution to nlohmann#464

jseward · 2017-10-21T23:38:37Z

Found some chat on #464 for similar issues. Hijacked the solution there for this new functionality.

coveralls · 2017-10-21T23:41:45Z

Coverage remained the same at 100.0% when pulling 1a66679 on jseward:develop into 33df325 on nlohmann:develop.

coveralls · 2017-10-22T00:46:48Z

Coverage remained the same at 100.0% when pulling d468f8c on jseward:develop into 33df325 on nlohmann:develop.

coveralls · 2017-10-22T01:54:12Z

Coverage remained the same at 100.0% when pulling d468f8c on jseward:develop into 33df325 on nlohmann:develop.

trilogy-service-ro · 2017-10-22T08:23:18Z

Can one of the admins verify this patch?

nlohmann · 2017-10-22T08:24:00Z

@trilogy-service-ro Please stop spamming PRs.

gregmarr · 2017-10-22T16:37:41Z

src/json.hpp

+		basic_json>>>;
+
+	using object_comparator_key_t = typename object_t::key_type;
+#endif


Any reason not to do the transparent comparator across the board for C++14 and higher?

gregmarr · 2017-10-22T16:38:47Z

src/json.hpp

@@ -10894,9 +10920,9 @@ class basic_json

    /*!
    @brief find an element in a JSON object
-    @copydoc find(typename object_t::key_type)
+    @copydoc find(object_transparent_comparator_key_t)


Where is this type (with transparent in it) defined? Just a typo here?

gregmarr · 2017-10-22T16:42:50Z

src/json.hpp

@@ -10929,7 +10955,7 @@ class basic_json

    @since version 1.0.0
    */
-    size_type count(typename object_t::key_type key) const
+    size_type count(object_comparator_key_t key) const


Would the find and count functions be better as templates with perfect forwarding, so we can find or count directly anything that the underlying object class can find or count?

@gregmarr

Instead implement @gregmarr's PR comments that perfect forwarding should be used. Also cleaned up cpp language standard detection.

jseward · 2017-10-23T07:56:38Z

@gregmarr thanks for the great feedback. I've implemented your suggested changes

theodelrieu · 2017-10-23T08:05:09Z

src/json.hpp

@@ -110,11 +110,11 @@ SOFTWARE.
 #endif

 // string_view support
-#if defined(_MSC_VER) && defined(_HAS_CXX17) && _HAS_CXX17 == 1
-	#define JSON_USE_STRING_VIEW
+#if (defined(__cplusplus) && __cplusplus >= 201703L) || (defined(_MSC_VER) && _MSC_VER > 1900 && defined(_HAS_CXX17) && _HAS_CXX17 == 1) // fix for issue #464


Could we use __has_include instead?

theodelrieu · 2017-10-23T08:09:48Z

src/json.hpp

+    @copydoc find(typename object_t::key_type)
+    */
+    template<typename KeyT>
+    iterator find(KeyT&& key)


Could we define those perfect-forwarded overloads for C++11 as well?

The templated member functions don't exist in 11. They were added in 14.

Yes, but we could keep those overloads in C++11, they will call the regular comparators which will perform implicit conversions, as for:

std::cout << std::multiplies<int>{}(2.0f, 2.0f) << std::endl;

I haven't tested my suggestion though, so it might very well be bogus.

My first thought was that it might be weird for users that aren't familiar with the difference in capabilities between std::map in 11 and 14 to have the templated type that only works with things implicitly convertible to the key type, but maybe that's not an issue, as you'd get the same issue with anything that isn't comparable with the key type with the 14 version. The STL kept the non-template find() and count(), but likely only for backwards compat. If it works, I think it would be better with just the templated versions. If we can't eliminate the fixed type versions, we should add const & to them to match the std::map versions. Right now we make a copy unnecessarily. (I just noticed this now.)

I like the suggestion, pushed changes to just have perfect-forwarding only

theodelrieu · 2017-10-23T08:11:32Z

test/CMakeLists.txt

+    if(MSVC_VERSION GREATER_EQUAL 1910)
+        # Enable c++17 support in Visual Studio 2017 (for testing perfect forwarding and transparent comparator in find() / count())
+        set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /std:c++17")
+    endif()


IIRC Appveyor already runs with VS2017, you might want to put that flag in the Appveyor file?

Indeed we have three configurations for AppVeyor:

MSVC 2015

MSVC 2017

MSVC 2017 + /permissive- /std:c++latest /utf-8

Ah nice, didn't realize that thanks. I'll this remove this then.

nlohmann · 2017-10-23T10:01:12Z

Sorry - AppVeyor fails because of a recent change of mine. I shall fix this.

nlohmann · 2017-10-23T10:33:34Z

The develop version now compiles without problems with AppVeyor.

coveralls · 2017-10-23T14:42:09Z

Coverage remained the same at 100.0% when pulling 33c6511 on jseward:develop into 33df325 on nlohmann:develop.

gregmarr · 2017-10-23T14:50:58Z

@jseward You're welcome. I'm glad it worked out.

Already covered by AppVeyor

coveralls · 2017-10-24T08:38:21Z

Coverage remained the same at 100.0% when pulling 992c836 on jseward:develop into 33df325 on nlohmann:develop.

coveralls · 2017-10-24T10:31:29Z

Coverage remained the same at 100.0% when pulling 992c836 on jseward:develop into 33df325 on nlohmann:develop.

coveralls · 2017-10-24T12:09:36Z

Coverage remained the same at 100.0% when pulling 992c836 on jseward:develop into 33df325 on nlohmann:develop.

jseward · 2017-10-25T06:41:23Z

travis-ci failing with:

==> Cloning https://github.com/nlohmann/json.git
Cloning into '/Users/travis/Library/Caches/Homebrew/nlohmann_json--git'...
remote: Counting objects: 1279, done.
remote: Compressing objects: 100% (605/605), done.
error: RPC failed; curl 56 SSLRead() return error -36
fatal: The remote end hung up unexpectedly
fatal: early EOF
fatal: index-pack failed
Error: Failed to download resource "nlohmann_json"
Failure while executing: git clone --depth 1 --branch develop https://github.com/nlohmann/json.git /Users/travis/Library/Caches/Homebrew/nlohmann_json--git
Error: Testing requires the latest version of nlohmann/json/nlohmann_json

don't notice an obvious way to retry

nlohmann · 2017-10-25T08:27:30Z

Travis' support for macOS is a bit flaky. I restarted the job.

nlohmann

Looks good to me.

nlohmann · 2017-10-27T09:47:29Z

Thanks a lot!

Add string_view support

1a66679

This avoids unnecessary string copies on often used find().

jseward changed the title ~~Add string_view support~~ Add std::string_view support Oct 21, 2017

jseward added 2 commits October 21, 2017 16:29

Fix for _HAS_CXX17 == 0

59cde1a

Copied from solution to nlohmann#464

Use consistent logic to determine if string_view exists

d468f8c

nlohmann mentioned this pull request Oct 22, 2017

Compilation Error on Clang 5.0 Upgrade #735

Closed

gregmarr reviewed Oct 22, 2017

View reviewed changes

Remove JSON_HAS_STRING_VIEW

33c6511

Instead implement @gregmarr's PR comments that perfect forwarding should be used. Also cleaned up cpp language standard detection.

jseward changed the title ~~Add std::string_view support~~ Add transparent comparator and perfect forwarding support to find() and count() Oct 23, 2017

jseward changed the title ~~Add transparent comparator and perfect forwarding support to find() and count()~~ Add transparent comparator and perfect forwarding support to find() and count() if CPP 14 is detected Oct 23, 2017

jseward changed the title ~~Add transparent comparator and perfect forwarding support to find() and count() if CPP 14 is detected~~ Add transparent comparator and perfect forwarding support to find() and count() Oct 23, 2017

theodelrieu reviewed Oct 23, 2017

View reviewed changes

jseward added 5 commits October 23, 2017 23:45

Remove tabs for spaces

73b1629

Remove c++17 support flag in cmake

16ffdbc

Already covered by AppVeyor

Remove old non-perfect forwarding find and count

6c9a401

Remove extra spaces

715c98b

Add missing spaces

992c836

jseward mentioned this pull request Oct 24, 2017

Add no thrown exception support to parse #798

Closed

nlohmann approved these changes Oct 27, 2017

View reviewed changes

nlohmann self-assigned this Oct 27, 2017

nlohmann added this to the Release 3.0.0 milestone Oct 27, 2017

nlohmann merged commit 77f8e2f into nlohmann:develop Oct 27, 2017

nlohmann added a commit that referenced this pull request Oct 27, 2017

📝 thanks for #795

7820b5e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add transparent comparator and perfect forwarding support to find() and count() #795

Add transparent comparator and perfect forwarding support to find() and count() #795

jseward commented Oct 21, 2017

jseward commented Oct 21, 2017

coveralls commented Oct 21, 2017

coveralls commented Oct 22, 2017

coveralls commented Oct 22, 2017

trilogy-service-ro commented Oct 22, 2017

nlohmann commented Oct 22, 2017

gregmarr Oct 22, 2017

gregmarr Oct 22, 2017 •

edited

Loading

gregmarr Oct 22, 2017

jseward commented Oct 23, 2017

theodelrieu Oct 23, 2017

theodelrieu Oct 23, 2017

gregmarr Oct 23, 2017

theodelrieu Oct 23, 2017

gregmarr Oct 23, 2017 •

edited

Loading

jseward Oct 24, 2017

theodelrieu Oct 23, 2017

nlohmann Oct 23, 2017

jseward Oct 24, 2017

nlohmann commented Oct 23, 2017

nlohmann commented Oct 23, 2017

coveralls commented Oct 23, 2017

gregmarr commented Oct 23, 2017

coveralls commented Oct 24, 2017

coveralls commented Oct 24, 2017

coveralls commented Oct 24, 2017

jseward commented Oct 25, 2017

nlohmann commented Oct 25, 2017

nlohmann left a comment

nlohmann commented Oct 27, 2017

Add transparent comparator and perfect forwarding support to find() and count() #795

Add transparent comparator and perfect forwarding support to find() and count() #795

Conversation

jseward commented Oct 21, 2017

jseward commented Oct 21, 2017

coveralls commented Oct 21, 2017

coveralls commented Oct 22, 2017

coveralls commented Oct 22, 2017

trilogy-service-ro commented Oct 22, 2017

nlohmann commented Oct 22, 2017

Choose a reason for hiding this comment

gregmarr Oct 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jseward commented Oct 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregmarr Oct 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nlohmann commented Oct 23, 2017

nlohmann commented Oct 23, 2017

coveralls commented Oct 23, 2017

gregmarr commented Oct 23, 2017

coveralls commented Oct 24, 2017

coveralls commented Oct 24, 2017

coveralls commented Oct 24, 2017

jseward commented Oct 25, 2017

nlohmann commented Oct 25, 2017

nlohmann left a comment

Choose a reason for hiding this comment

nlohmann commented Oct 27, 2017

gregmarr Oct 22, 2017 •

edited

Loading

gregmarr Oct 23, 2017 •

edited

Loading