Skip to content

Commit

Permalink
Address review comments
Browse files Browse the repository at this point in the history 
Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>

Co-authored-by: Johan Mabille <johan.mabille@gmail.com>
  • Loading branch information
@jjerphan @JohanMabille
jjerphan and JohanMabille committed Mar 26, 2024
1 parent bfa28ac commit adf6d86
Show file tree
Hide file tree
Showing 3 changed files with 85 additions and 26 deletions.
14 changes: 7 additions & 7 deletions include/sparrow/dynamic_bitset.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ namespace sparrow
size_type size() const noexcept;
size_type null_count() const noexcept;

bool test(size_type pos) const;
const_reference test(size_type pos) const;
void set(size_type pos, value_type value);

reference operator[](size_type i);
Expand Down Expand Up @@ -221,7 +221,7 @@ namespace sparrow
void assign(bool) noexcept;
void set() noexcept;
void reset() noexcept;

bitset_type& m_bitset;
block_type& m_block;
block_type m_mask;
Expand Down Expand Up @@ -274,7 +274,7 @@ namespace sparrow
mpl::constify_t<typename B::value_type, is_const>,
std::contiguous_iterator_tag,
std::conditional_t<is_const, bool, bitset_reference<B>>
>;
>;
using reference = typename base_type::reference;
using difference_type = typename base_type::difference_type;

Expand All @@ -297,7 +297,7 @@ namespace sparrow

friend class iterator_access;
};

/**************************************
* dynamic_bitset_base implementation *
**************************************/
Expand Down Expand Up @@ -631,7 +631,7 @@ namespace sparrow
}
return *this;
}

template <class B>
auto bitset_reference<B>::operator|=(bool rhs) noexcept -> self_type&
{
Expand All @@ -641,7 +641,7 @@ namespace sparrow
}
return *this;
}

template <class B>
auto bitset_reference<B>::operator^=(bool rhs) noexcept -> self_type&
{
Expand Down Expand Up @@ -708,7 +708,7 @@ namespace sparrow
{
assert(m_index < bitset_type::s_bits_per_block);
}

template <class B, bool is_const>
auto bitset_iterator<B, is_const>::dereference() const -> reference
{
Expand Down
61 changes: 45 additions & 16 deletions include/sparrow/fixed_size_layout.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ namespace sparrow
* @tparam T The type of the elements in the layout's data buffer.
* @tparam is_const A boolean indicating whether the iterator is const.
*
* @note This class is not thread-safe, exception-safe, copyable, movable, equality comparable.
* @note This class is copyable and movable.
*/
template <class T, bool is_const>
class fixed_size_layout_value_iterator
Expand Down Expand Up @@ -66,7 +66,7 @@ namespace sparrow
void increment();
void decrement();
void advance(difference_type n);
difference_type distance_to(const self_type& rhs) const;
difference_type distance_to(const self_type& hs) const;
bool equal(const self_type& rhs) const;
bool less_than(const self_type& rhs) const;

Expand Down Expand Up @@ -94,9 +94,12 @@ namespace sparrow

using self_type = fixed_size_layout<T>;
using inner_value_type = T;
using value_type = std::optional<inner_value_type>;
using inner_reference = inner_value_type&;
using inner_const_reference = const inner_reference;
using bitmap_type = array_data::bitmap_type;
using bitmap_reference = typename bitmap_type::reference;
using bitmap_const_reference = typename bitmap_type::const_reference;
using value_type = std::optional<inner_value_type>;
using reference = reference_proxy<self_type>;
using const_reference = const_reference_proxy<self_type>;
using pointer = inner_value_type*;
Expand All @@ -113,6 +116,9 @@ namespace sparrow
using const_bitmap_range = std::ranges::subrange<const_bitmap_iterator>;
using const_value_range = std::ranges::subrange<const_value_iterator>;

using bitmap_range = std::ranges::subrange<bitmap_iterator>;
using value_range = std::ranges::subrange<value_iterator>;

// TODO: implement with `begin` and `end` once the iterator is available.
// using iterator = reference_proxy<self_type>::iterator;
// using const_iterator = const_reference_proxy<self_type>::iterator;
Expand All @@ -124,6 +130,9 @@ namespace sparrow
reference operator[](size_type i);
const_reference operator[](size_type i) const;

bitmap_range bitmap();
value_range values();

const_bitmap_range bitmap() const;
const_value_range values() const;

Expand All @@ -134,7 +143,8 @@ namespace sparrow
pointer data();
const_pointer data() const;

bool has_value(size_type i) const;
bitmap_reference has_value(size_type i);
bitmap_const_reference has_value(size_type i) const;
inner_reference value(size_type i);
inner_const_reference value(size_type i) const;

Expand Down Expand Up @@ -197,22 +207,22 @@ namespace sparrow
template <class T, bool is_const>
bool fixed_size_layout_value_iterator<T, is_const>::equal(const self_type& rhs) const
{
return distance_to(rhs) == 0;
return rhs.m_pointer == m_pointer;
}

template <class T, bool is_const>
bool fixed_size_layout_value_iterator<T, is_const>::less_than(const self_type& rhs) const
{
return distance_to(rhs) > 0;
return m_pointer < rhs.m_pointer;
}

/************************************
* fixed_size_layout implementation *
* *********************************/
***********************************/

template <class T>
fixed_size_layout<T>::fixed_size_layout(array_data ad)
: m_data(ad)
: m_data(std::move(ad))
{
// We only require the presence of the bitmap and the first buffer.
assert(m_data.buffers.size() > 0);
Expand Down Expand Up @@ -245,14 +255,26 @@ namespace sparrow
auto fixed_size_layout<T>::operator[](size_type i) -> reference
{
assert(i < size());
return reference(*this, i);
return reference(value(i), has_value(i));
}

template <class T>
auto fixed_size_layout<T>::operator[](size_type i) const -> const_reference
{
assert(i < size());
return const_reference(*this, i);
return const_reference(value(i), has_value(i));
}

template <class T>
auto fixed_size_layout<T>::bitmap() -> bitmap_range
{
return std::ranges::subrange(bitmap_begin(), bitmap_end());
}

template <class T>
auto fixed_size_layout<T>::values() -> value_range
{
return std::ranges::subrange(value_begin(), value_end());
}

template <class T>
Expand All @@ -268,10 +290,17 @@ namespace sparrow
}

template <class T>
auto fixed_size_layout<T>::has_value(size_type i) const -> bool
auto fixed_size_layout<T>::has_value(size_type i) -> bitmap_reference
{
assert(i < size());
return m_data.bitmap[i];
}

template <class T>
auto fixed_size_layout<T>::has_value(size_type i) const -> bitmap_const_reference
{
assert(i < size());
return m_data.bitmap.test(i);
return m_data.bitmap[i];
}

template <class T>
Expand All @@ -283,7 +312,7 @@ namespace sparrow
template <class T>
auto fixed_size_layout<T>::value_end() -> value_iterator
{
return value_iterator{data() + self_type::size()};
return value_iterator{data() + size()};
}

template <class T>
Expand All @@ -295,7 +324,7 @@ namespace sparrow
template <class T>
auto fixed_size_layout<T>::value_cend() const -> const_value_iterator
{
return const_value_iterator{data() + self_type::size()};
return const_value_iterator{data() + size()};
}

template <class T>
Expand All @@ -307,7 +336,7 @@ namespace sparrow
template <class T>
auto fixed_size_layout<T>::bitmap_end() -> bitmap_iterator
{
return m_data.bitmap.begin() + self_type::size();
return m_data.bitmap.begin() + size();
}

template <class T>
Expand All @@ -319,7 +348,7 @@ namespace sparrow
template <class T>
auto fixed_size_layout<T>::bitmap_cend() const -> const_bitmap_iterator
{
return m_data.bitmap.cbegin() + self_type::size();
return m_data.bitmap.cbegin() + size();
}

template <class T>
Expand Down
36 changes: 33 additions & 3 deletions test/test_layout.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ namespace sparrow
array_data ad;

ad.type = data_descriptor(data_type::UINT8);
ad.bitmap = dynamic_bitset<uint8_t>(n);
ad.bitmap = dynamic_bitset<uint8_t>(n, true);
buffer<uint8_t> b(n);
std::iota(b.begin(), b.end(), 0);

Expand All @@ -55,9 +55,39 @@ namespace sparrow
array_data ad = make_test_array_data();
layout_test_type lt(ad);
REQUIRE(lt.size() == ad.length);

for (std::size_t i = 0; i < lt.size(); ++i)
{
CHECK_EQ(lt[i].value(), ad.buffers[0][i]);
}
}

TEST_CASE("value_iterator_ordering")
{
layout_test_type lt(make_test_array_data());
auto lt_values = lt.values();
layout_test_type::value_iterator iter = lt_values.begin();
// TODO: Allow coercion of iterator to const_iterator.
// layout_test_type::const_value_iterator citer = lt_values.begin();
REQUIRE(iter < lt_values.end());
// REQUIRE(citer < lt_values.end());
}
}

// TODO: Test the iterators once they are implemented.
TEST_CASE("value_iterator_equality")
{
layout_test_type lt(make_test_array_data());
auto lt_values = lt.values();
layout_test_type::value_iterator iter = lt_values.begin();
// TODO: Allow coercion of iterator to const_iterator.
// layout_test_type::const_value_iterator citer = lt_values.begin();
for (std::size_t i = 0; i < lt.size(); ++i)
{
CHECK_EQ(*iter++, lt[i]);
// CHECK_EQ(*citer++, lt[i]);
}
CHECK_EQ(iter, lt_values.end());
// CHECK_EQ(citer, lt_values.end());
}

}
}

0 comments on commit adf6d86

Please sign in to comment.