Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding Structured Binding Support #1388

Closed
pratikpc opened this issue Dec 9, 2018 · 8 comments
Closed

Adding Structured Binding Support #1388

pratikpc opened this issue Dec 9, 2018 · 8 comments

Comments

@pratikpc
Copy link
Contributor

pratikpc commented Dec 9, 2018

  • Describe the feature in as much detail as possible.
    Given the fact that the Json Library now implements items() function, utilising Structured Bindings in this scenario would make the code much more simpler, easier to read and understand
    Even though Structured Bindings were added in C++17, the components/engines which ensure it exists are present in C++11 and C++14 and adding support would not require any #ifdef

  • Include sample usage where appropriate.
    A Sample usage of Structured Binding is

   // example for an object
   for (auto& [key,value] : j_object.items())
   {
      std::cout << "key: " << key << ", value: " << value << '\n';
   }
  • Implementation
    The reason I am writing it as an issue rather than as a pull request is because it makes one questionable change.
    Most of the implementation is referenced from TartanLlama's Blog

    In the Class iteration_proxy_internal we would have to add the function get for Structured Bindings Support.
    Thanks for @theodelrieu 's recommendations which helped shorten the code
// Required for Structured Bindings support
template<std::size_t N, typename = typename std::enable_if<N >= 2>::type>
void get()const
{
   static_assert(false, "Has Max 2 Elements:- Key or Value");
}
template<std::size_t N, typename = typename  std::enable_if<N == 0>::type>
auto get() const -> decltype(key())
{
   return key();
}
template<std::size_t N, typename = typename std::enable_if<N == 1>::type>
auto get() const  -> decltype(value())
{
   return value();
}

This is not a major change and would cause no incompatibility.
Adding support for Structured Bindings however involves minor editing of the std namespace which may or may not be favoured by a few projects.

namespace std {
   template<std::size_t N>
   struct tuple_element<N, nlohmann::detail::iteration_proxy<nlohmann::json::iterator>::iteration_proxy_internal> {
      using type = decltype(std::declval<nlohmann::detail::iteration_proxy<nlohmann::json::iterator>::iteration_proxy_internal>().get<N>());
   };

   template<>
   struct tuple_size<nlohmann::detail::iteration_proxy<nlohmann::json::iterator>::iteration_proxy_internal>
      : std::integral_constant<std::size_t, 2> {};
}

This is where we face an issue.
In both these methods, iteration_proxy_internal needs to be accessible.
However, as we know, iterator_proxy_internal is defined as a private method in the iterator_proxy class.
Hence in order to add support for Structured Bindings, we would have to either make it public, find a hack or use something similar to friend
This is also the reason I decided to submit it as an issue rather than make a pull request.

I apologise for any and all mistakes committed by me while writing this.
I am quite new to GitHub and this is one of my first feature requests on a project

EDIT: Thanks to @theodelrieu for making recommendations which helped shorten the code

@theodelrieu
Copy link
Contributor

I agree this would be nice to have, you should consider opening a PR :)

A few remarks:

I do not see why a new namespace is needed.

About the access level, there is no need to change it, get can be added as a non-member friend template function.

@pratikpc
Copy link
Contributor Author

pratikpc commented Dec 10, 2018

No the issue is not with get.
The issue is with tuple_element's template parameter requiring iteration_proxy_internal which itself is private.

struct tuple_element<N, nlohmann::detail::iteration_proxy<nlohmann::json::iterator>::iteration_proxy_internal> {

I had a feeling there might have been some reason why it was kept private and hence wanted to discuss and confirm the same.
Which was one of the major reasons why I wanted to communicate with the community involved in the project before sending the PR.

Thank you so much for pointing out that the namespaces were unnecessary.
I was utilising it as I think at one point I was using something similar to get<0> and get<1> which can be added within a class using MSVC but fails when using GCC (go figure :-P)
Later I wised up and decided to use enable_if

If it's okay with you @theodelrieu I am going to edit the main comment to recognise the code that does not anymore require the namespaces and put the get within the class.

@theodelrieu
Copy link
Contributor

The issue is with tuple_element's template parameter requiring iteration_proxy_internal which itself is private.

items return a iteration_proxy<iterator>, so I do not think iteration_proxy_internal has to be exposed at all, you can just implement all the required methods with iteration_proxy<iterator> instead.

I'm just guessing right now, did not play with structured bindings yet. But I think this should not be as hard to implement

@pratikpc
Copy link
Contributor Author

pratikpc commented Dec 10, 2018

@theodelrieu while iterator_proxy is a very nicely implemented class, one of the issues it again faces is that all it does is returns the begin and the end.
It does not for example, store the values or keep the key and the value

The key and the value are present in iteration_proxy_internal which is a source of a minor issue

Also for example let us write the code

   for (auto const elem : parse.items())
   {

   }

In this scenario, elem will be of the type iterator_proxy_internal which means it is somewhat getting exposed( or so Visual Studio/EDG tells me)

Coming back to key and value, those are the two most important facets regarding structured bindings.

However while discussing with you I had a Eureka moment and I am still confused why I did not think about it before

      public:
         // Required for supporting Structured Bindings
         using internal_proxy_for_binding = iteration_proxy_internal;

This leaks the code out and makes it available for tuple_size and tuple_element.
(I am not sure if it's a feature or a bug of the C++ Standard)

As this stands, especially with the lack of requirement to drop Private, would this be good enough to make a PR Request??

@theodelrieu
Copy link
Contributor

I agree that having private in this case is not very useful, the class is already a member of detail.

Plus, the API is exposed, only the name is not, which can always be retrieved via decltype().
I'm for the removal of private, the structured bindings support is an implementation detail as well, so I do not see the need to hide stuff.

As this stands, especially with the lack of requirement to drop Private, would this be good enough to make a PR Request??

Yup, I think we're good to go :)

@pratikpc
Copy link
Contributor Author

@theodelrieu I seem to be facing some issue

struct tuple_element<N, nlohmann::detail::iteration_proxynlohmann::json::iterator::iteration_proxy_internal>

See when I am modifying the Multiple Include files, I decided to place the get functions where they should have been in iterators/iteration_proxy.hpp

However, the std function have a further requirement of needing the parameter nlohmann::json::iterator
So should I forward declare it or add it within json.hpp( I think that's comparatively a more nicer solution)??

Or is there some template magic you know?

@theodelrieu
Copy link
Contributor

You could forward declare it, or simply partially specialize:

namespace std {
template <typename T, std::size_t N>
struct tuple_element<N, nlohmann::detail::iteration_proxy<T>>{ /* ... */ };
}

@pratikpc
Copy link
Contributor Author

No that was the first thing I tried.
That didn't work.
Templates are so powerful and yet so weird at the same time.

@pratikpc pratikpc changed the title Adding Structured Bidning Support Adding Structured Binding Support Dec 10, 2018
@nlohmann nlohmann self-assigned this Dec 20, 2018
@nlohmann nlohmann added this to the Release 3.4.1 milestone Dec 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants