Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

###The above points applied to your code:

The above points applied to your code:

###The above points applied to your code:

The above points applied to your code:

Defaulted copies are memberwise (though that's same as bitwise here).
Source Link
  • You should provide the pre- and post-increment operators (++it and it++). Currently, you only have the pre-increment version.

  • It might also be good to provide the -> operator, since some people prefer the it->something syntax over the (*it).something one.

  • The comparison and dereference operators should be const. Remember Const Correctness.

  • The copy constructor is just performing a bitwisememberwise copy of the underlying data, so you don't need to provide one and can let the compiler default it.

  • The Standard Library containers always provide two flavors of iterators, the iterator type, pointing to mutable data, and the const_iterator type, pointing to immutable data. It is easy to adapt your class to support both by providing a conversion operator and inheriting from std::iterator (see the following example).

  • Decide which course of action should be taken when incrementing and dereferencing an invalid iterator. E.g.: list.end()++;. Should it trigger an assertion? Throw an exception? Do nothing as it is now? I would at least assert to help the debugging process. You might find exceptions more appropriate in your context.

  • You should provide the pre- and post-increment operators (++it and it++). Currently, you only have the pre-increment version.

  • It might also be good to provide the -> operator, since some people prefer the it->something syntax over the (*it).something one.

  • The comparison and dereference operators should be const. Remember Const Correctness.

  • The copy constructor is just performing a bitwise copy of the underlying data, so you don't need to provide one and can let the compiler default it.

  • The Standard Library containers always provide two flavors of iterators, the iterator type, pointing to mutable data, and the const_iterator type, pointing to immutable data. It is easy to adapt your class to support both by providing a conversion operator and inheriting from std::iterator (see the following example).

  • Decide which course of action should be taken when incrementing and dereferencing an invalid iterator. E.g.: list.end()++;. Should it trigger an assertion? Throw an exception? Do nothing as it is now? I would at least assert to help the debugging process. You might find exceptions more appropriate in your context.

  • You should provide the pre- and post-increment operators (++it and it++). Currently, you only have the pre-increment version.

  • It might also be good to provide the -> operator, since some people prefer the it->something syntax over the (*it).something one.

  • The comparison and dereference operators should be const. Remember Const Correctness.

  • The copy constructor is just performing a memberwise copy of the underlying data, so you don't need to provide one and can let the compiler default it.

  • The Standard Library containers always provide two flavors of iterators, the iterator type, pointing to mutable data, and the const_iterator type, pointing to immutable data. It is easy to adapt your class to support both by providing a conversion operator and inheriting from std::iterator (see the following example).

  • Decide which course of action should be taken when incrementing and dereferencing an invalid iterator. E.g.: list.end()++;. Should it trigger an assertion? Throw an exception? Do nothing as it is now? I would at least assert to help the debugging process. You might find exceptions more appropriate in your context.

Changed code formatting.
Source Link
glampert
  • 17.3k
  • 4
  • 31
  • 89
#include <cassert> // assert
#include <cstddef> // ptrdiff_t
#include <iterator> // iterator
#include <type_traits> // remove_cv
#include <utility> // swap
template<class Type,template
<
 class Type,
 class UnqualifiedType = std::remove_cv_t<Type>>remove_cv_t<Type>
>
class ForwardIterator 
 : public std::iterator<std::forward_iterator_tag,
 UnqualifiedType,
 std::ptrdiff_t,
 Type*,
 Type&>
{
 node<UnqualifiedType>* itr;
 explicit ForwardIterator(node<UnqualifiedType>* nd) 
 : itr(nd) 
 { 
 }
public:
 ForwardIterator() // Default construct gives end.
 : itr(nullptr) 
 { 
 }
 void swap(ForwardIterator& other) noexcept
 {
 using std::swap;
 swap(itr, other.iter);
 }
 ForwardIterator& operator++ () // Pre-increment
 {
 assert(itr != nullptr && "Out-of-bounds iterator increment!");
 itr = itr->next;
 return *this;
 }
 ForwardIterator operator++ (int) // Post-increment
 {
 assert(itr != nullptr && "Out-of-bounds iterator increment!");
 ForwardIterator tmp(*this);
 itr = itr->next;
 return tmp; 
 }
 // two-way comparison: v.begin() == v.cbegin() and vice versa
 template<class OtherType>
 bool operator == (const ForwardIterator<OtherType>& rhs) const
 {
 return itr == rhs.itr;
 }
 
 template<class OtherType>
 bool operator != (const ForwardIterator<OtherType>& rhs) const
 {
 return itr != rhs.itr;
 }
 Type& operator* () const
 {
 assert(itr != nullptr && "Invalid iterator dereference!");
 return itr->data;
 }
 Type& operator-> () const
 {
 assert(itr != nullptr && "Invalid iterator dereference!");
 return itr->data;
 }
 // One way conversion: iterator -> const_iterator
 operator ForwardIterator<const Type>() const
 {
 return ForwardIterator<const Type>(itr);
 }
};
// `iterator` and `const_iterator` used by your class:
typedef ForwardIterator<T> iterator;
typedef ForwardIterator<const T> const_iterator;
#include <cassert> // assert
#include <cstddef> // ptrdiff_t
#include <iterator> // iterator
#include <type_traits> // remove_cv
#include <utility> // swap
template<class Type,
 class UnqualifiedType = std::remove_cv_t<Type>>
class ForwardIterator 
 : public std::iterator<std::forward_iterator_tag,
 UnqualifiedType,
 ptrdiff_t,
 Type*,
 Type&>
{
 node<UnqualifiedType>* itr;
 explicit ForwardIterator(node<UnqualifiedType>* nd) 
 : itr(nd) 
 { 
 }
public:
 ForwardIterator() // Default construct gives end.
 : itr(nullptr) 
 { 
 }
 void swap(ForwardIterator& other) noexcept
 {
 using std::swap;
 swap(itr, other.iter);
 }
 ForwardIterator& operator++ () // Pre-increment
 {
 assert(itr != nullptr && "Out-of-bounds iterator increment!");
 itr = itr->next;
 return *this;
 }
 ForwardIterator operator++ (int) // Post-increment
 {
 assert(itr != nullptr && "Out-of-bounds iterator increment!");
 ForwardIterator tmp(*this);
 itr = itr->next;
 return tmp; 
 }
 // two-way comparison: v.begin() == v.cbegin() and vice versa
 template<class OtherType>
 bool operator == (const ForwardIterator<OtherType>& rhs) const
 {
 return itr == rhs.itr;
 }
 
 template<class OtherType>
 bool operator != (const ForwardIterator<OtherType>& rhs) const
 {
 return itr != rhs.itr;
 }
 Type& operator* () const
 {
 assert(itr != nullptr && "Invalid iterator dereference!");
 return itr->data;
 }
 Type& operator-> () const
 {
 assert(itr != nullptr && "Invalid iterator dereference!");
 return itr->data;
 }
 // One way conversion: iterator -> const_iterator
 operator ForwardIterator<const Type>() const
 {
 return ForwardIterator<const Type>(itr);
 }
};
// `iterator` and `const_iterator` used by your class:
typedef ForwardIterator<T> iterator;
typedef ForwardIterator<const T> const_iterator;
#include <cassert> // assert
#include <cstddef> // ptrdiff_t
#include <iterator> // iterator
#include <type_traits> // remove_cv
#include <utility> // swap
template
<
 class Type,
 class UnqualifiedType = std::remove_cv_t<Type>
>
class ForwardIterator 
 : public std::iterator<std::forward_iterator_tag,
 UnqualifiedType,
 std::ptrdiff_t,
 Type*,
 Type&>
{
 node<UnqualifiedType>* itr;
 explicit ForwardIterator(node<UnqualifiedType>* nd) 
 : itr(nd) 
 { 
 }
public:
 ForwardIterator() // Default construct gives end.
 : itr(nullptr) 
 { 
 }
 void swap(ForwardIterator& other) noexcept
 {
 using std::swap;
 swap(itr, other.iter);
 }
 ForwardIterator& operator++ () // Pre-increment
 {
 assert(itr != nullptr && "Out-of-bounds iterator increment!");
 itr = itr->next;
 return *this;
 }
 ForwardIterator operator++ (int) // Post-increment
 {
 assert(itr != nullptr && "Out-of-bounds iterator increment!");
 ForwardIterator tmp(*this);
 itr = itr->next;
 return tmp; 
 }
 // two-way comparison: v.begin() == v.cbegin() and vice versa
 template<class OtherType>
 bool operator == (const ForwardIterator<OtherType>& rhs) const
 {
 return itr == rhs.itr;
 }
 
 template<class OtherType>
 bool operator != (const ForwardIterator<OtherType>& rhs) const
 {
 return itr != rhs.itr;
 }
 Type& operator* () const
 {
 assert(itr != nullptr && "Invalid iterator dereference!");
 return itr->data;
 }
 Type& operator-> () const
 {
 assert(itr != nullptr && "Invalid iterator dereference!");
 return itr->data;
 }
 // One way conversion: iterator -> const_iterator
 operator ForwardIterator<const Type>() const
 {
 return ForwardIterator<const Type>(itr);
 }
};
// `iterator` and `const_iterator` used by your class:
typedef ForwardIterator<T> iterator;
typedef ForwardIterator<const T> const_iterator;
fix bugs and typos throughout (but the answer is basically correct, and deserves its upvotes)
Source Link
Loading
added 12 characters in body
Source Link
glampert
  • 17.3k
  • 4
  • 31
  • 89
Loading
Replaced 'nothrow' with 'noexcept'.
Source Link
glampert
  • 17.3k
  • 4
  • 31
  • 89
Loading
added 258 characters in body
Source Link
Loki Astari
  • 97.7k
  • 5
  • 126
  • 341
Loading
Source Link
glampert
  • 17.3k
  • 4
  • 31
  • 89
Loading
lang-cpp

AltStyle によって変換されたページ (->オリジナル) /