HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

AdjacentElementsIteratorAdaptor for Random Access Iterators

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
randomadjacentelementsiteratoradaptoriteratorsforaccess

Problem

Inspired by this comment I decided to implement an iterator adaptor that takes another iterator and returns each element and a given number of its neighbors.

The final solution should be applicable to most iterator types but this review is only about my first solution for random access iterators and can be found here.

adjacent_element_iterator.hpp

```
#ifndef ADJACENT_ELEMENT_ITERATOR_HPP_
#define ADJACENT_ELEMENT_ITERATOR_HPP_

#include
#include
#include
#include

template
struct AdjacentElementRandomAccessIterator;

/**
* @brief Helps dereferencing into a tuple of the given size
*/
template
struct DereferenceHelper {
template
static auto dereference(WrappedIterator const ¤t,
Arguments... arguments)
-> decltype(DereferenceHelper::dereference(current,
std::forward(
arguments)...,
current)) {
auto next = std::next(current);
return DereferenceHelper::dereference(next,
std::forward(
arguments)...,
next);
}
};

/**
* @brief Ends the recursion and actually creates the tuple
*/
template
struct DereferenceHelper {
template
static auto dereference(WrappedIterator const & /current/,
Arguments... arguments)
-> decltype(std::tie(*arguments...)) {
return std::tie(*arguments...);
}
};

/**
* @brief Shorthand for a NumberOfElements tuple of dereference WrappedIterators
*/
template
using AdjacentElementValue =
decltype(DereferenceHelper::dereference(
std::declval(), std::declval()));

/**
* @brief An iterator adaptor that returns NumberOfElements neighboring elements
* at once
*/
template
struct AdjacentElementRandomAccessIterator
: boost::it

Solution

Generally speaking, the code is really well-written and looks as idiomatic as it could possibly be. That's probably why you didn't get any answer since you asked it. So... well, I will make a small and highly subjective review since I still can't see any obvious problem with your code:

-
It seems that Boost and the standard library do things in subtly different ways when it comes to iterators. The standard library iterator adapters use explicit constructors to transform iterators when the constructors have only one parameter while Boost does not seem to follow the practice. You might want to make AdjacentElementRandomAccessIterator's constructor explicit.

On the other hand, it means that you won't be able to use the nice syntax return { / ... / } when construct-returning an instance of AdjacentElementRandomAccessIterator.

-
Yet another difference: the standard library tends to pass iterators by value while Boost tends to pass them by const reference. It seems that you followed the Boost way to do things. I can't say which is better but you may want to think again about it and change or not your decision.

-
I generally don't talk about case because that's often a matter of taste, but when writing library components that look like those you can find in Boost or the standard library, I find it better to use the snake_case so that it looks more consistent. The mixed case of make_AdjacentElementIterator looks especially bad to my eye :/

-
You may want to only use lower case letters for the static_assert error message, which is the case compilers tend to use for their diagnostics.

As you can see, that's as subjective a review as you will get. If I try to comment further, I will talk more and more about style (and there is nothing wrong with your style, so that would be pointless) or about C++14 but your code seems to explicitly target C++11 so that would be pointless too.

Context

StackExchange Code Review Q#84996, answer score: 3

Revisions (0)

No revisions yet.