Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Simplified slice #2674

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Simplified slice #2674

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 13 additions & 26 deletions include/exiv2/slice.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ struct ConstSliceBase : SliceBase {
if (new_end > this->end_) {
throw std::out_of_range("Invalid input parameters to slice");
}
return slice_type(storage_.data_, new_begin, new_end);
return {storage_.data_, new_begin, new_end};
}

protected:
Expand Down Expand Up @@ -176,21 +176,21 @@ struct MutableSliceBase : public ConstSliceBase<storage_type, data_type> {
return this->storage_.unsafeAt(this->begin_ + index);
}

const value_type& at(size_t index) const {
[[nodiscard]] const value_type& at(size_t index) const {
return base_type::at(index);
}

/*!
* Obtain an iterator to the first element in the slice.
*/
iterator begin() noexcept {
[[nodiscard]] iterator begin() noexcept {
return this->storage_.unsafeGetIteratorAt(this->begin_);
}

/*!
* Obtain an iterator to the first element beyond the slice.
*/
iterator end() noexcept {
[[nodiscard]] iterator end() noexcept {
return this->storage_.unsafeGetIteratorAt(this->end_);
}

Expand Down Expand Up @@ -227,7 +227,7 @@ struct MutableSliceBase : public ConstSliceBase<storage_type, data_type> {
* mutable_slice_base.
*/
template <typename slice_type>
slice_type subSlice(size_t begin, size_t end) {
[[nodiscard]] slice_type subSlice(size_t begin, size_t end) {
this->rangeCheck(begin);
// end == size() is a legal value, since end is the first
// element beyond the slice
Expand All @@ -241,7 +241,7 @@ struct MutableSliceBase : public ConstSliceBase<storage_type, data_type> {
if (new_end > this->end_) {
throw std::out_of_range("Invalid input parameters to slice");
}
return slice_type(this->storage_.data_, new_begin, new_end);
return {this->storage_.data_, new_begin, new_end};
}
};

Expand Down Expand Up @@ -429,24 +429,11 @@ struct Slice : public Internal::MutableSliceBase<Internal::ContainerStorage, con
using value_type = typename std::remove_cv<typename container::value_type>::type;
#endif

/*!
* Construct a sub-slice of this slice with the given bounds. The bounds
* are evaluated with respect to the current slice.
*
* @param[in] begin First element in the new slice.
* @param[in] end First element beyond the new slice.
*
* @throw std::out_of_range when begin or end are invalid
*/
Slice subSlice(size_t begin, size_t end) {
return Internal::MutableSliceBase<Internal::ContainerStorage, container>::template subSlice<Slice>(begin, end);
}

/*!
* Constructs a new constant subSlice. Behaves otherwise exactly like
* the non-const version.
*/
[[nodiscard]] Slice<const container> subSlice(size_t begin, size_t end) const {
Slice<const container> subSlice(size_t begin, size_t end) const {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you remove [[nodiscard]]?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if I don't, it starts warning on the unit tests.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to fix that in the unit tests. Could you suppress the warning with the cast-to-void trick?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eh I don't see the value. All such warnings are in ASSERT_THROW

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. Fixed the unit tests.

return this->to_const_base().template subSlice<Slice<const container>>(begin, end);
}
};
Expand Down Expand Up @@ -530,23 +517,23 @@ struct Slice<T*> : public Internal::MutableSliceBase<Internal::PtrSliceStorage,
* parameter deduction.
*/
template <typename T>
Slice<T> makeSlice(T& cont, size_t begin, size_t end) {
[[nodiscard]] Slice<T> makeSlice(T& cont, size_t begin, size_t end) {
return {cont, begin, end};
}

/*!
* Overload of makeSlice for slices of C-arrays.
*/
template <typename T>
Slice<T*> makeSlice(T* ptr, size_t begin, size_t end) {
[[nodiscard]] Slice<T*> makeSlice(T* ptr, size_t begin, size_t end) {
return {ptr, begin, end};
}

/*!
* @brief Return a new slice spanning the whole container.
*/
template <typename container>
Slice<container> makeSlice(container& cont) {
[[nodiscard]] Slice<container> makeSlice(container& cont) {
return {cont, 0, cont.size()};
}

Expand All @@ -555,23 +542,23 @@ Slice<container> makeSlice(container& cont) {
* container.
*/
template <typename container>
Slice<container> makeSliceFrom(container& cont, size_t begin) {
[[nodiscard]] Slice<container> makeSliceFrom(container& cont, size_t begin) {
return {cont, begin, cont.size()};
}

/*!
* @brief Return a new slice spanning until `end`.
*/
template <typename container>
Slice<container> makeSliceUntil(container& cont, size_t end) {
[[nodiscard]] Slice<container> makeSliceUntil(container& cont, size_t end) {
return {cont, 0, end};
}

/*!
* Overload of makeSliceUntil for pointer based slices.
*/
template <typename T>
Slice<T*> makeSliceUntil(T* ptr, size_t end) {
[[nodiscard]] Slice<T*> makeSliceUntil(T* ptr, size_t end) {
return {ptr, 0, end};
}

Expand Down
28 changes: 11 additions & 17 deletions unitTests/test_slice.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,20 +97,18 @@ TYPED_TEST_P(slice, constructionFailsWithZeroLength) {
* Test the construction of subSlices and their behavior.
*/
TYPED_TEST_P(slice, subSliceSuccessfulConstruction) {
using slice_t = Slice<TypeParam>;

// 0 1 2 3 4 5 6 7 8 9
// | | center_vals
// | | middle
slice_t center_vals = this->getTestSlice(3, 7);
auto center_vals = this->getTestSlice(3, 7);
ASSERT_EQ(center_vals.size(), static_cast<size_t>(4));
ASSERT_NO_THROW(center_vals.subSlice(1, 3));

ASSERT_NO_THROW(center_vals.subSlice(1, center_vals.size()));
}

TYPED_TEST_P(slice, subSliceFunctions) {
Slice<TypeParam> middle = this->getTestSlice(3, 7).subSlice(1, 3);
auto middle = this->getTestSlice(3, 7).subSlice(1, 3);

ASSERT_EQ(middle.size(), static_cast<size_t>(2));
ASSERT_EQ(middle.at(1), static_cast<typename Slice<TypeParam>::value_type>(5));
Expand Down Expand Up @@ -163,11 +161,9 @@ void checkSubSlice(const Slice<T>& sl) {
* Test that all slices can be also passed as const references and still work
*/
TYPED_TEST_P(slice, constMethodsPreserveConst) {
using slice_t = Slice<TypeParam>;

// 0 1 2 3 4 5 6 7 8 9
// | | center_vals
slice_t center_vals = this->getTestSlice(3, 7);
auto center_vals = this->getTestSlice(3, 7);

// check at() const works
checkConstSliceValueAt(center_vals, 4, 1);
Expand All @@ -181,30 +177,28 @@ TYPED_TEST_P(slice, constMethodsPreserveConst) {
* Test the non-const iterators
*/
TYPED_TEST_P(mutableSlice, iterators) {
using slice_t = Slice<TypeParam>;
slice_t sl = this->getTestSlice();
auto sl = this->getTestSlice();

ASSERT_EQ(*sl.begin(), static_cast<typename slice_t::value_type>(1));
ASSERT_EQ(*sl.end(), static_cast<typename slice_t::value_type>(this->vec_size - 1));
ASSERT_EQ(*sl.begin(), static_cast<typename decltype(sl)::value_type>(1));
ASSERT_EQ(*sl.end(), static_cast<typename decltype(sl)::value_type>(this->vec_size - 1));

for (auto it = sl.begin(); it < sl.end(); ++it) {
*it = 2 * (*it);
*it *= 2;
}

ASSERT_EQ(this->vec_.at(0), 0);
for (size_t j = 1; j < this->vec_size - 1; ++j) {
ASSERT_EQ(this->vec_.at(j), static_cast<typename slice_t::value_type>(2 * j));
ASSERT_EQ(this->vec_.at(j), static_cast<typename decltype(sl)::value_type>(2 * j));
ASSERT_EQ(this->vec_.at(j), sl.at(j - 1));
}
ASSERT_EQ(this->vec_.at(this->vec_size - 1), static_cast<typename slice_t::value_type>(this->vec_size - 1));
ASSERT_EQ(this->vec_.at(this->vec_size - 1), static_cast<typename decltype(sl)::value_type>(this->vec_size - 1));
}

/*!
* Test the non-const version of at()
*/
TYPED_TEST_P(mutableSlice, at) {
using slice_t = Slice<TypeParam>;
slice_t sl = this->getTestSlice(2, 4);
auto sl = this->getTestSlice(2, 4);

sl.at(0) = 6;
sl.at(1) = 12;
Expand All @@ -215,7 +209,7 @@ TYPED_TEST_P(mutableSlice, at) {
if (j == 2 || j == 3) {
continue;
}
ASSERT_EQ(this->vec_.at(j), static_cast<typename slice_t::value_type>(j));
ASSERT_EQ(this->vec_.at(j), static_cast<typename decltype(sl)::value_type>(j));
}
}

Expand Down
Loading