patterncppMinor
Am I using C++11 features like STL and move semantics correctly?
Viewed 0 times
stlsemanticslikemoveusingandfeaturescorrectly
Problem
I've implemented selection sort, heap sort, insertion sort, and iterative quicksort using C++11. Am I using the correct STL data structures/algorithms? Am I using move semantics correctly in my code?
```
#include
#include
#include
#include
#include
#include
#include
namespace detail
{
template
void heapSort(It begin, It end, Comp compFunc, std::random_access_iterator_tag)
{
std::make_heap(begin, end, compFunc);
std::sort_heap(begin, end, compFunc);
}
template
void heapSort(It begin, It end, Comp compFunc, IterCat)
{
typedef typename It::value_type value_type;
std::vector randomAccessContainer(std::make_move_iterator(begin), std::make_move_iterator(end));
heapSort(std::begin(randomAccessContainer), std::end(randomAccessContainer), compFunc, std::random_access_iterator_tag());
std::move(std::begin(randomAccessContainer), std::end(randomAccessContainer), begin);
}
template
class QuicksortStack
{
private:
typedef std::pair IterRange;
std::stack m_stack;
public:
void push(IterRange&& range)
{
if (std::distance(range.first, range.second) >= 2)
{
m_stack.push(std::move(range));
}
}
void push(const IterRange& range)
{
if (std::distance(range.first, range.second) >= 2)
{
m_stack.push(range);
}
}
void pop()
{
m_stack.pop();
}
IterRange top() const
{
return m_stack.top();
}
bool empty() const
{
return m_stack.empty();
}
};
}
template
void selectionSort(It begin, It end, Comp compFunc)
{
for (; begin != end; ++begin)
{
const It minElem(std::min_element(begin, end, compFunc));
if (begin != minElem)
{
std::iter_swap(begin, minElem);
```
#include
#include
#include
#include
#include
#include
#include
namespace detail
{
template
void heapSort(It begin, It end, Comp compFunc, std::random_access_iterator_tag)
{
std::make_heap(begin, end, compFunc);
std::sort_heap(begin, end, compFunc);
}
template
void heapSort(It begin, It end, Comp compFunc, IterCat)
{
typedef typename It::value_type value_type;
std::vector randomAccessContainer(std::make_move_iterator(begin), std::make_move_iterator(end));
heapSort(std::begin(randomAccessContainer), std::end(randomAccessContainer), compFunc, std::random_access_iterator_tag());
std::move(std::begin(randomAccessContainer), std::end(randomAccessContainer), begin);
}
template
class QuicksortStack
{
private:
typedef std::pair IterRange;
std::stack m_stack;
public:
void push(IterRange&& range)
{
if (std::distance(range.first, range.second) >= 2)
{
m_stack.push(std::move(range));
}
}
void push(const IterRange& range)
{
if (std::distance(range.first, range.second) >= 2)
{
m_stack.push(range);
}
}
void pop()
{
m_stack.pop();
}
IterRange top() const
{
return m_stack.top();
}
bool empty() const
{
return m_stack.empty();
}
};
}
template
void selectionSort(It begin, It end, Comp compFunc)
{
for (; begin != end; ++begin)
{
const It minElem(std::min_element(begin, end, compFunc));
if (begin != minElem)
{
std::iter_swap(begin, minElem);
Solution
Your code looks good, I don't see any major problems.
Here's what I would consider changing:
....
Edit:
In hindsight, that insertion sort is sub optimal. Yours will be quite bad too. Here's a much faster one, with different STL use:
Edit2:
Your forwarding functions (the functions that fill in a default compare function) don't allow for raw pointer iterators. The STL algorithms do. The problem is that you're using
Here's what I would consider changing:
- Name your iterator template arguments by the iterator concept they are required to meet. For example:
template
- I would prefer not to see early
returns from functions if possible. I think it's clearer to doif(blah){ do stuff }then to doif(!blah){ return; } do stuff
- I didn't look closely at all of them, but you can use STL to make a much prettier insertion sort:
....
template
void InsertionSort(ForwardIt first, ForwardIt last, Compare comp)
{
for(auto i = first; i != last; ++i)
{
std::rotate(std::upper_bound(first, i, *i, comp), i, std::next(i));
}
}Edit:
In hindsight, that insertion sort is sub optimal. Yours will be quite bad too. Here's a much faster one, with different STL use:
template
void InsertionSort(BiDirIt first, BiDirIt last, Compare comp)
{
for(auto i = first; i != last; ++i)
{
auto current = std::move(*i);
auto start = std::upper_bound(first, i, current, comp);
std::move_backward(start, i, std::next(i));
*start = std::move(current);
}
}Edit2:
Your forwarding functions (the functions that fill in a default compare function) don't allow for raw pointer iterators. The STL algorithms do. The problem is that you're using
typename FwdIt::value_type. Obviously a raw pointer doesn't have a member typedef called value_type. The way to solve this is to use iterator_traits:template
void selectionSort(FwdIt begin, FwdIt end)
{
selectionSort(begin, end, std::less::value_type>());
}Code Snippets
template<typename ForwardIt, typename Compare>
void InsertionSort(ForwardIt first, ForwardIt last, Compare comp)
{
for(auto i = first; i != last; ++i)
{
std::rotate(std::upper_bound(first, i, *i, comp), i, std::next(i));
}
}template<typename BiDirIt, typename Compare>
void InsertionSort(BiDirIt first, BiDirIt last, Compare comp)
{
for(auto i = first; i != last; ++i)
{
auto current = std::move(*i);
auto start = std::upper_bound(first, i, current, comp);
std::move_backward(start, i, std::next(i));
*start = std::move(current);
}
}template <typename FwdIt>
void selectionSort(FwdIt begin, FwdIt end)
{
selectionSort(begin, end, std::less<typename std::iterator_traits<FwdIt>::value_type>());
}Context
StackExchange Code Review Q#24548, answer score: 5
Revisions (0)
No revisions yet.