Replace class iterator and const_iterator by using a single template class to reduce code.#395
Replace class iterator and const_iterator by using a single template class to reduce code.#395nlohmann merged 2 commits intonlohmann:developfrom Bosswestfalen:develop
Conversation
…late <typename U> iter_impl. iter_impl has operator const_iterator to create an const_iterator from an iterator.
nlohmann
left a comment
nlohmann
left a comment
There was a problem hiding this comment.
If the tests succeed, I am very impressed! I added some questions where I think the code can be even more refined.
| static_assert(std::is_same<U, basic_json>::value | ||
| or std::is_same<U, const basic_json>::value, | ||
| "iter_impl only accepts (const) basic_json"); | ||
|
|
There was a problem hiding this comment.
Why not move this into the template specification std::enable_if<...>?
There was a problem hiding this comment.
When U is not (const) basic_json this will give a more informative error message. Unfortunately you do not notice this requirement just by looking at the head of the class.
In fact it can be moved to the specification.
There was a problem hiding this comment.
As the iterators are only defined inside the basic_json class, I think it's OK to skip the error message and move it to the specification. The code could maybe even simplified by using std::is_same<U, std::remove_cv<basic_json>>.
There was a problem hiding this comment.
Should be std::is_same<std::remove_cv<U>, basic_json>. But yes, this looks much better than my 3 lines above.
| using difference_type = typename basic_json::difference_type; | ||
| /// defines a pointer to the type iterated over (value_type) | ||
| using pointer = typename basic_json::const_pointer; | ||
| using pointer = typename std::conditional<std::is_const<U>::value, |
There was a problem hiding this comment.
Can't you use typename U::pointer?
There was a problem hiding this comment.
I am not familiar with allocators. My understanding is that std::allocator_traits<allocator_type>::pointer; (line 260) using AllocatorType</* no const */ basic_json> (line 257), is never const. So I did not test it. If I#m wrong I'd prefer typename U::pointer.
There was a problem hiding this comment.
You were right: const_iterator should have a const pointer, see http://en.cppreference.com/w/cpp/iterator/iterator_traits.
| typename basic_json::pointer>::type; | ||
| /// defines a reference to the type iterated over (value_type) | ||
| using reference = typename basic_json::const_reference; | ||
| using reference = typename std::conditional<std::is_const<U>::value, |
There was a problem hiding this comment.
Same as above - why not use typename U::reference?
|
Thanks! |
3 participants
Class iterator and const_iterator can be merged by a single template class.
Currently the classes differ by aliases. This can be handled by the template parameter.
Const_iterator can be created using an iterator. Iterator cannot be created from a const_iterator.
This can be achieved by a conversion function in the template class.