Added Support for Structured Bindings #1391

pratikpc · 2018-12-10T15:24:08Z

Closes #1388.

[Describe your pull request here. Please read the text below the line, and make sure you follow the checklist.]

   for (auto const& [key,value] : json.items())
   {
      std::cout << "key: " << key << ", value: " << value << '\n';
   }

For further details, read #1388 and https://blog.tartanllama.xyz/structured-bindings/

Structured Bindings help simplify writing code and using this library greatly especially given the way they are implemented in C++17 and the addition of the items function.
The method of implementation ensures that Structured Bindings implementation can also be added in projects using C++11 with only C++17 being able to reap the benefits.

Pull request checklist

Read the Contribution Guidelines for detailed information.

Changes are described in the pull request, or an existing issue is referenced.
The test suite compiles and runs without error.
Code coverage is 100%. Test cases can be added by editing the test suite.
The source code is amalgamated; that is, after making changes to the sources in the include/nlohmann directory, run make amalgamate to create the single-header file single_include/nlohmann/json.hpp. The whole process is described here.

Please don't

The C++11 support varies between different compilers and versions. Please note the list of supported compilers. Some compilers like GCC 4.7 (and earlier), Clang 3.3 (and earlier), or Microsoft Visual Studio 13.0 and earlier are known not to work due to missing or incomplete C++11 support. Please refrain from proposing changes that work around these compiler's limitations with #ifdefs or other means.
Specifically, I am aware of compilation problems with Microsoft Visual Studio (there even is an issue label for these kind of bugs). I understand that even in 2016, complete C++11 support isn't there yet. But please also understand that I do not want to drop features or uglify the code just to make Microsoft's sub-standard compiler happy. The past has shown that there are ways to express the functionality such that the code compiles with the most recent MSVC - unfortunately, this is not the main objective of the project.
Please refrain from proposing changes that would break JSON conformance. If you propose a conformant extension of JSON to be supported by the library, please motivate this extension.
Please do not open pull requests that address multiple issues.

theodelrieu · 2018-12-10T15:28:44Z

include/nlohmann/detail/iterators/iteration_proxy.hpp

+    // We would have to make iteration_proxy_internal
+    // Accessible to tuple_size and tuple_element in std namespace
+    // However, as iteration_proxy_internal is private,
+    // We will have to use a Hack to add Structured Bindings Support


I thought we agreed on making iteration_proxy_internal public?

I made the necessary changes

I do not have much of a clue how LGTM or AppVeyor work so do they restart with the new commit or would I have to create a new one??

You do not have to create a new one IIRC

@theodelrieu thanks!
That's actually a nice feature

include/nlohmann/detail/iterators/iteration_proxy.hpp

coveralls · 2018-12-10T22:11:46Z

Coverage remained the same at 100.0% when pulling ebd3f45 on pratikpc:develop into 4f270e3 on nlohmann:develop.

theodelrieu · 2018-12-11T13:41:21Z

I just pushed a commit on my fork which is still a wip, but the basic use-case works (with non-member get functions).

I had to move out iteration_proxy_internal outside of iteration_proxy to be able to partially specialize tuple_size and tuple_element (otherwise, only nlohmann::json would support structured bindings, not different instances of nlohmann::basic_json<...>).

There might be some refactoring to be done with iteration_proxy/_internal, I'd like to know what @nlohmann thinks about it, I do not know that part of the code very well.

Also, I have not tested edge-cases yet...

@pratikpc What do you think about incorporating my changes in your PR? We can work out the remaining details if you want :)

pratikpc · 2018-12-13T06:42:54Z

@pratikpc What do you think about incorporating my changes in your PR? We can work out the remaining details if you want :)

That's a superb change.
I am updating my repo to bring it in line with these changes.

In the commit we can possibly push the get function to the std namespace as well

namespace std
{
   // structured bindings support
   template <std::size_t N, typename IteratorType, enable_if_t<N == 0, int> = 0>
   auto get(const nlohmann::detail::iteration_proxy_internal<IteratorType>& i) -> decltype(i.key())
   {
      return i.key();
   }

   template <std::size_t N, typename IteratorType, enable_if_t<N == 1, int> = 0>
   auto get(const nlohmann::detail::iteration_proxy_internal<IteratorType>& i) -> decltype(i.value())
   {
      return i.value();
   }
}

Making it public and independent of iteration_proxy kills of all the problems we were facing.
I am all for it.
Although I still believe we need to know @nlohmann 's views regarding that.
Should we rename iteration_proxy_internal now?????

pratikpc · 2018-12-13T08:46:35Z

@theodelrieu the commit broke a Unit Test.
Specifically the test-regression

One way to correct it is to add a Constructor in Basic Json class which accepts iteration_proxy_internal as an argument

template<typename Iterator>
basic_json(detail::iteration_proxy_internal<Iterator> other) : basic_json{ other.value() }
{}

However, my belief is that this is a temporary fix

The error I faced was
[2018-12-13 07:41:19] [build] /usr/include/c++/7/bits/stl_algo.h:5337:16: error: no match for ‘operator=’ (operand types are ‘std::insert_iterator<nlohmann::basic_json<> >’ and ‘nlohmann::detail::iteration_proxy_internal<nlohmann::detail::iter_impl<nlohmann::basic_json<> > >’)
Also faced here at LGTM

theodelrieu · 2018-12-13T09:17:17Z

Oops, my bad, I only tested unit-items.cpp......

In the commit we can possibly push the get function to the std namespace as well

That's illegal unfortunately, the get methods are where they should be, i.e. in the same namespace as the class.

I will make the code work on my box and let you know when I push!

pratikpc · 2018-12-13T09:20:13Z

That's illegal unfortunately, the get methods are where they should be, i.e. in the same namespace as the class.

Oh.
I thought it would be fine if they were kept in std namespace.
Because Structured Bindings still works on GCC and MSVC

I will make the code work on my box and let you know when I push!

The unit-tests now no longer fail after adding the Constructor.
But yes, finding a more permanent solution would be a good idea for us all.

I would add a commit which shifts get back into the namespace as before

theodelrieu · 2018-12-13T09:40:36Z

I just push-forced on my fork, the fix was in conversions/to_json.hpp as you can see.
I've added a test with a custom basic_json alias as well, if you have other tests in mind do not hesitate to add them :)

I thought it would be fine if they were kept in std namespace.

You can see the rules from cppreference

pratikpc · 2018-12-13T09:51:57Z

@theodelrieu I can't find the changes.
Did you make them at https://github.com/theodelrieu/json/blob/feature/structured_bindings/include/nlohmann/detail/conversions/to_json.hpp

EDIT
Modified with BugFix for Regression Failure.
It seems like the implementation for Structured Bindings in the Library is complete

Should iteration_proxy_internal be renamed @theodelrieu ?
Considering it's no longer internal?

theodelrieu · 2018-12-13T10:18:06Z

It is still in namespace detail, so the current name does not annoy me that much. Need @nlohmann's input on this one.

I do think the PR is complete, could you squash your commits?

pratikpc · 2018-12-13T11:36:28Z

@theodelrieu how does one squash a commit exactly???

theodelrieu · 2018-12-13T11:42:17Z

First, do not forget to run make amalgamate and commit the result.

Once this is done, run git rebase -i develop. It will open a prompt.
Then, keep the first line as-is, and replace pick in all the others by fixup.
Save and quit, then git push --force.

pratikpc · 2018-12-13T12:05:59Z

@theodelrieu thanks!!!

pratikpc · 2018-12-14T02:55:20Z

@theodelrieu The following tests FAILED:
80 - test-unicode_all (Timeout)
On Appveyor
Does this mean we have to start the process again?

nlohmann · 2018-12-14T08:49:05Z

I’ll restart.

nlohmann · 2018-12-14T09:15:44Z

Appveyor seems to work now. :)

pratikpc · 2018-12-14T09:27:18Z

@nlohmann is it okay that we have made iteration_proxy_internal public and brought it outside iteration_proxy within the detail namespace?
Do we need to change it's name or anything?

nlohmann · 2018-12-15T07:52:07Z

I am traveling right now, so I won’t be able to check before next week. But anything that brings us closer toward support of structured bindings is greatly appreciated!

include/nlohmann/detail/iterators/iteration_proxy.hpp

include/nlohmann/detail/conversions/to_json.hpp

include/nlohmann/detail/iterators/iteration_proxy.hpp

For further details, read #1388 and https://blog.tartanllama.xyz/structured-bindings/

nlohmann

Looks good to me.

nlohmann · 2018-12-20T07:58:16Z

Thanks so much!

theodelrieu reviewed Dec 10, 2018

View reviewed changes

include/nlohmann/detail/iterators/iteration_proxy.hpp Show resolved Hide resolved

nlohmann requested changes Dec 16, 2018

View reviewed changes

include/nlohmann/detail/iterators/iteration_proxy.hpp Show resolved Hide resolved

include/nlohmann/detail/conversions/to_json.hpp Show resolved Hide resolved

include/nlohmann/detail/iterators/iteration_proxy.hpp Outdated Show resolved Hide resolved

nlohmann approved these changes Dec 19, 2018

View reviewed changes

Added Support for Structured Bindings

ebd3f45

For further details, read #1388 and https://blog.tartanllama.xyz/structured-bindings/

nlohmann approved these changes Dec 20, 2018

View reviewed changes

nlohmann self-assigned this Dec 20, 2018

nlohmann added the release item: ✨ new feature label Dec 20, 2018

nlohmann added this to the Release 3.4.1 milestone Dec 20, 2018

nlohmann merged commit 8584994 into nlohmann:develop Dec 20, 2018

This was referenced Dec 20, 2018

Range-based iteration over a non-array object #998

Closed

No structured bindings support? #901

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Support for Structured Bindings #1391

Added Support for Structured Bindings #1391

pratikpc commented Dec 10, 2018 •

edited by nlohmann

Loading

theodelrieu Dec 10, 2018

pratikpc Dec 10, 2018

pratikpc Dec 10, 2018

theodelrieu Dec 10, 2018

pratikpc Dec 10, 2018

coveralls commented Dec 10, 2018 •

edited

Loading

theodelrieu commented Dec 11, 2018

pratikpc commented Dec 13, 2018 •

edited

Loading

pratikpc commented Dec 13, 2018

theodelrieu commented Dec 13, 2018

pratikpc commented Dec 13, 2018 •

edited

Loading

theodelrieu commented Dec 13, 2018

pratikpc commented Dec 13, 2018 •

edited

Loading

theodelrieu commented Dec 13, 2018

pratikpc commented Dec 13, 2018

theodelrieu commented Dec 13, 2018

pratikpc commented Dec 13, 2018

pratikpc commented Dec 14, 2018

nlohmann commented Dec 14, 2018

nlohmann commented Dec 14, 2018

pratikpc commented Dec 14, 2018

nlohmann commented Dec 15, 2018

nlohmann left a comment

nlohmann commented Dec 20, 2018

Added Support for Structured Bindings #1391

Added Support for Structured Bindings #1391

Conversation

pratikpc commented Dec 10, 2018 • edited by nlohmann Loading

Pull request checklist

Please don't

theodelrieu Dec 10, 2018

Choose a reason for hiding this comment

pratikpc Dec 10, 2018

Choose a reason for hiding this comment

pratikpc Dec 10, 2018

Choose a reason for hiding this comment

theodelrieu Dec 10, 2018

Choose a reason for hiding this comment

pratikpc Dec 10, 2018

Choose a reason for hiding this comment

coveralls commented Dec 10, 2018 • edited Loading

theodelrieu commented Dec 11, 2018

pratikpc commented Dec 13, 2018 • edited Loading

pratikpc commented Dec 13, 2018

theodelrieu commented Dec 13, 2018

pratikpc commented Dec 13, 2018 • edited Loading

theodelrieu commented Dec 13, 2018

pratikpc commented Dec 13, 2018 • edited Loading

theodelrieu commented Dec 13, 2018

pratikpc commented Dec 13, 2018

theodelrieu commented Dec 13, 2018

pratikpc commented Dec 13, 2018

pratikpc commented Dec 14, 2018

nlohmann commented Dec 14, 2018

nlohmann commented Dec 14, 2018

pratikpc commented Dec 14, 2018

nlohmann commented Dec 15, 2018

nlohmann left a comment

Choose a reason for hiding this comment

nlohmann commented Dec 20, 2018

pratikpc commented Dec 10, 2018 •

edited by nlohmann

Loading

coveralls commented Dec 10, 2018 •

edited

Loading

pratikpc commented Dec 13, 2018 •

edited

Loading

pratikpc commented Dec 13, 2018 •

edited

Loading

pratikpc commented Dec 13, 2018 •

edited

Loading