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

reverse_iterator operator inheritance problem #93

Closed
TmsKtel opened this issue Jun 18, 2015 · 14 comments
Closed

reverse_iterator operator inheritance problem #93

TmsKtel opened this issue Jun 18, 2015 · 14 comments

Comments

@TmsKtel
Copy link

TmsKtel commented Jun 18, 2015

int main()
{
      json a = {1, 2, 3};
      json::reverse_iterator rit = a.rbegin();
      ++rit;
      return 0;
}

gives:
error: ‘std::reverse_iterator<_Iterator>& std::reverse_iterator<_Iterator>::operator++() [with _Iterator = nlohmann::basic_json<>::iterator]’ is inaccessible
with g++
and
error: 'operator++' is a private member of 'std::reverse_iterator<nlohmann::basic_json<std::map, std::vector, std::basic_string, bool,
long, double, std::allocator>::iterator>'
with clang++

@nlohmann nlohmann self-assigned this Jun 19, 2015
nlohmann added a commit that referenced this issue Jun 19, 2015
@nlohmann
Copy link
Owner

Thanks for the hint - this was a stupid mistake...

@TmsKtel
Copy link
Author

TmsKtel commented Jun 19, 2015

Thank You for Your quick reaction, this solves the problem mentioned above.
However, it seems, that ++ returns an std::reverse_iterator, and not a json::reverse_iterator.

int main()
{
    json a = {1,2,3};
    json::reverse_iterator rit = ++a.rbegin();
    return 0;
}

trying to compile the above code gives the following error:
error: no viable conversion from 'std::reverse_iterator<nlohmann::basic_json<std::map, std::vector, std::basic_string, bool, long, double, std::allocator>::iterator>' to
'json::reverse_iterator

@nlohmann nlohmann reopened this Jun 19, 2015
@nlohmann
Copy link
Owner

This is strange.

int main()
{
    json a = {1,2,3};
    json::reverse_iterator rit = a.rbegin();
    ++rit;
    return 0;
}

works... I'll check.

@TmsKtel
Copy link
Author

TmsKtel commented Jun 22, 2015

int main()
{
    json a = {1,2,3};
    json::reverse_iterator rit = a.rbegin();
    ++rit;
    json b = {0,0,0};
    std::transform(rit,a.rend(),b.rbegin(),[](json el){return el;});
    std::cout<<b <<std::endl;
    return 0;
}

compiles, and the output is : [0,1,2]
but trying to compile

int main()
{
    json a = {1,2,3};
    json b = {0,0,0};
    std::transform(++a.rbegin(),a.rend(),b.rbegin(),[](json el){return el;});
    std::cout<<b <<std::endl;
    return 0;
}

I get this error:
error: no matching function for call to 'transform'
stl_algo.h:4915:5: note: candidate template ignored: deduced conflicting types for parameter '_InputIterator'
('std::reverse_iterator<...>' vs. 'nlohmann::basic_json<...>::reverse_iterator')

@nlohmann
Copy link
Owner

Interestingly, all code mentioned here compiles if you comment out the classes reverse_iterator and const_reverse_iterator and change the forward declarations in the beginning of json.hpp to

/// a reverse iterator for a basic_json container
using reverse_iterator = std::reverse_iterator<iterator>;
/// a const reverse iterator for a basic_json container
using const_reverse_iterator = std::reverse_iterator<const_iterator>;

However, the functions key() and value() which are available for iterator and const_iterator are not available to reverse_iterator or const_iterator. And I have no idea how to add them, because implementing the classes reverse_iterator and const_reverse_iterator yielded this issue... Any ideas?

@nlohmann
Copy link
Owner

@gregmarr
Copy link
Contributor

When creating your reverse_iterator derived class, you should only need to implement those members that return std::reverse_iterator and make them return your own iterator type instead.

@nlohmann
Copy link
Owner

Hi @gregmarr, unfortunately, I do not understand what you mean. I described my problem at http://stackoverflow.com/questions/31036364/inherit-all-functions-from-user-defined-iterator-in-reverse-iterator - maybe you can help?

@gregmarr
Copy link
Contributor

The only way I see to do it is something like this:

using mytype = reverse_iterator;
using mybase = std::reverse_iterator;

mytype &operator++()
{
mybase::operator++();
return *this;
}

mytype operator++(int)
{
return mytype(mybase::operator++(1));
}

and repeating that for these:
operator+=
operator+ (both member and non-member)
operator--
operator--(int)
operator-=
operator-

@nlohmann nlohmann added this to the Release 3.0 RC milestone Jul 1, 2015
@nlohmann
Copy link
Owner

nlohmann commented Jul 6, 2015

Ok, I could fix #100. The problem was that I needed to decrement base() before forwarding calls to it.

I made little progress in this issue, but still have problems with in-place usage of operator++ and operator++(int): There seems to be a problem converting std::reverse_iterator<json::iterator> to json::reverse_iterator, and I have no idea how to fix this...

@gregmarr , maybe you have an idea...

Cheers
Niels

@gregmarr
Copy link
Contributor

gregmarr commented Jul 7, 2015

What code do you have that isn't working? I won't be able to look at it until Sunday due to vacation, but I can try then.

@nlohmann
Copy link
Owner

nlohmann commented Jul 7, 2015

In commit 19d550c, I implemented a reverse_iterator and const_reverse_iterator which allows to compile all code in this issue (also added it as unit test).

The code is not as pretty as I hoped (I am still waiting for answers on Statck Overflow), but it seems OK. I would be glad for any feedback (especially @gregmarr whose hints were very valuable!) on the commit. I especially dislike the fact that the const and non-const versions share a lot code, and the reverse versions are also quite generic.

@nlohmann nlohmann closed this as completed Jul 7, 2015
@gregmarr
Copy link
Contributor

I'm having trouble building unit.cpp under VS2015, and I'm not sure why, but this should give you a single implementation for both reverse_iterator and const_reverse_iterator. There were also some member functions in there that looked like they would be the same as the base class version, so I'm not sure if they're necessary. Feel free to change any names that you don't like.

template<typename Base> class json_reverse_iterator : public std::reverse_iterator<Base>
{
  public:
    /// shortcut to the reverse iterator adaptor
    using base_iterator = std::reverse_iterator<T>;

    /// create reverse iterator from iterator
    json_reverse_iterator(const typename base_iterator::iterator_type& it)
        : base_iterator(it) {}

    /// create reverse iterator from base class
    json_reverse_iterator(const base_iterator& it) : base_iterator(it) {}

    /// post-increment (it++)
    json_reverse_iterator operator++(int)
    {
        return base_iterator::operator++(1);
    }

    /// pre-increment (++it)
    json_reverse_iterator& operator++()
    {
        base_iterator::operator++();
        return *this;
    }

    /// post-decrement (it--)
    json_reverse_iterator operator--(int)
    {
        return base_iterator::operator--(1);
    }

    /// pre-decrement (--it)
    json_reverse_iterator& operator--()
    {
        base_iterator::operator--();
        return *this;
    }

    /// add to iterator
    json_reverse_iterator& operator+=(difference_type i)
    {
        base_iterator::operator+=(i);
        return *this;
    }

    /// add to iterator
    json_reverse_iterator operator+(difference_type i) const
    {
        auto result = *this;
        result += i;
        return result;
    }

    /// subtract from iterator
    json_reverse_iterator operator-(difference_type i) const
    {
        auto result = *this;
        result -= i;
        return result;
    }
};
using reverse_iterator = json_reverse_iterator<typename basic_json::iterator>;
using const_reverse_iterator = json_reverse_iterator<typename basic_json::const_iterator>;

@nlohmann
Copy link
Owner

Thanks for the code. I'll check!

nlohmann added a commit that referenced this issue Jul 14, 2015
Iterators are now implemented via const_iterators and reverse_iterator
and const_reverse_iterator now share a template class. Thanks a lot!
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