Address review comments

Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>

Co-authored-by: Johan Mabille <johan.mabille@gmail.com>

include/sparrow/fixed_size_layout.hpp

@@ -120,8 +120,8 @@ namespace sparrow
  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;
+ // using iterator = layout_iterator<self_type, false>;
+ // using const_iterator = layout_iterator<self_type, true>;
 
  explicit fixed_size_layout(array_data p);
 
@@ -226,29 +226,28 @@ namespace sparrow
  {
  // We only require the presence of the bitmap and the first buffer.
  assert(m_data.buffers.size() > 0);
- assert(m_data.length == m_data.buffers[0].size());
  assert(m_data.length == m_data.bitmap.size());
  }
 
  template <class T>
  auto fixed_size_layout<T>::size() const -> size_type
  {
- assert(m_data.buffers.size() > 0);
- return m_data.buffers[0].size();
+ assert(m_data.offset <= m_data.length);
+ return static_cast<size_type>(m_data.length - m_data.offset);
  }
 
  template <class T>
  auto fixed_size_layout<T>::value(size_type i) -> inner_reference
  {
  assert(i < size());
- return data()[i];
+ return data()[i + m_data.offset];
  }
 
  template <class T>
  auto fixed_size_layout<T>::value(size_type i) const -> inner_const_reference
  {
  assert(i < size());
- return data()[i];
+ return data()[i + m_data.offset];
  }
 
  template <class T>
@@ -293,76 +292,76 @@ namespace sparrow
  auto fixed_size_layout<T>::has_value(size_type i) -> bitmap_reference
  {
  assert(i < size());
- return m_data.bitmap[i];
+ return m_data.bitmap[i + m_data.offset];
  }
 
  template <class T>
  auto fixed_size_layout<T>::has_value(size_type i) const -> bitmap_const_reference
  {
  assert(i < size());
- return m_data.bitmap[i];
+ return m_data.bitmap[i + m_data.offset];
  }
 
  template <class T>
  auto fixed_size_layout<T>::value_begin() -> value_iterator
  {
- return value_iterator{data()};
+ return value_iterator{data() + m_data.offset};
  }
 
  template <class T>
  auto fixed_size_layout<T>::value_end() -> value_iterator
  {
- return value_iterator{data() + size()};
+ return value_begin() + size();
  }
 
  template <class T>
  auto fixed_size_layout<T>::value_cbegin() const -> const_value_iterator
  {
- return const_value_iterator{data()};
+ return const_value_iterator{data() + m_data.offset};
  }
 
  template <class T>
  auto fixed_size_layout<T>::value_cend() const -> const_value_iterator
  {
- return const_value_iterator{data() + size()};
+ return value_cbegin() + size();
  }
 
  template <class T>
  auto fixed_size_layout<T>::bitmap_begin() -> bitmap_iterator
  {
- return m_data.bitmap.begin();
+ return m_data.bitmap.begin() + m_data.offset;
  }
 
  template <class T>
  auto fixed_size_layout<T>::bitmap_end() -> bitmap_iterator
  {
- return m_data.bitmap.begin() + size();
+ return bitmap_begin() + size();
  }
 
  template <class T>
  auto fixed_size_layout<T>::bitmap_cbegin() const -> const_bitmap_iterator
  {
- return m_data.bitmap.cbegin();
+ return m_data.bitmap.cbegin() + m_data.offset;
  }
 
  template <class T>
  auto fixed_size_layout<T>::bitmap_cend() const -> const_bitmap_iterator
  {
- return m_data.bitmap.cbegin() + size();
+ return bitmap_cbegin() + size();
  }
 
  template <class T>
  auto fixed_size_layout<T>::data() -> pointer
  {
  assert(m_data.buffers.size() > 0);
- return m_data.buffers[0].data();
+ return m_data.buffers[0].template data<inner_value_type>();
  }
 
  template <class T>
  auto fixed_size_layout<T>::data() const -> const_pointer
  {
  assert(m_data.buffers.size() > 0);
- return m_data.buffers[0].data();
+ return m_data.buffers[0].template data<inner_value_type>();
  }
 
 } // namespace sparrow

test/test_layout.cpp

@@ -25,23 +25,23 @@ namespace sparrow
  // static_assert(std::contiguous_iterator<fixed_size_layout_iterator<uint8_t, true>>);
  // static_assert(std::contiguous_iterator<fixed_size_layout_iterator<uint8_t, false>>);
 
- using layout_test_type = fixed_size_layout<uint8_t>;
+ using data_type_t = int32_t;
+ using layout_test_type = fixed_size_layout<data_type_t>;
 
  namespace
  {
  array_data make_test_array_data()
  {
  size_t n = 10;
  array_data ad;
-
- ad.type = data_descriptor(data_type::UINT8);
+ ad.type = data_descriptor(data_type::INT32);
  ad.bitmap = dynamic_bitset<uint8_t>(n, true);
- buffer<uint8_t> b(n);
- std::iota(b.begin(), b.end(), 0);
-
+ size_t buffer_size = (n * sizeof(data_type_t)) / sizeof(uint8_t);
+ buffer<uint8_t> b(buffer_size);
+ std::iota(b.data<int32_t>(), b.data<int32_t>() + n, -8);
  ad.buffers.push_back(b);
  ad.length = n;
- ad.offset = 0;
+ ad.offset = 1;
  ad.child_data.push_back(array_data());
  return ad;
  }
@@ -54,11 +54,12 @@ namespace sparrow
  {
  array_data ad = make_test_array_data();
  layout_test_type lt(ad);
- REQUIRE(lt.size() == ad.length);
+ CHECK_EQ(lt.size(), ad.length - ad.offset);
 
+ auto buffer_data = ad.buffers[0].data<data_type_t>();
  for (std::size_t i = 0; i < lt.size(); ++i)
  {
- CHECK_EQ(lt[i].value(), ad.buffers[0][i]);
+ CHECK_EQ(lt[i].value(), buffer_data[i + ad.offset]);
  }
  }
 
@@ -90,4 +91,5 @@ namespace sparrow
  }
 
  }
+
 }