Fix some vector issues.
Destructors are called too early which can be problematic when doing a push_back with an element of the vector.
Change-Id: I3a93d8f6783aa9a579d1d8dab2f454520df39622
Reviewed-on: https://chromium-review.googlesource.com/c/codecs/libwebp2/+/4251881
Tested-by: WebM Builds <builds@webmproject.org>
Reviewed-by: Maryla Ustarroz-Calonge <maryla@google.com>
diff --git a/src/utils/vector.h b/src/utils/vector.h
index e1fb4d1..e51501e 100644
--- a/src/utils/vector.h
+++ b/src/utils/vector.h
@@ -62,6 +62,7 @@
template <typename T>
class VectorBase {
public:
+ typedef T value_type;
typedef T* iterator;
typedef const T* const_iterator;
@@ -76,30 +77,15 @@
// Reallocates just enough memory if needed so that 'new_cap' items can fit.
WP2_NO_DISCARD bool reserve(size_t new_cap) {
if (capacity_ < new_cap) {
- // Not using realloc() because only 'num_items_' need to be moved.
T* const new_items = (T*)WP2Malloc(new_cap, sizeof(T));
if (new_items == nullptr) return false;
- if (num_items_ > 0) {
- if (std::is_trivially_copyable<T>::value &&
- std::is_trivially_destructible<T>::value) {
- std::memcpy((void*)new_items, (const void*)items_,
- num_items_ * sizeof(T));
- } else {
- for (size_t i = 0; i < num_items_; ++i) {
- new (&new_items[i]) T(std::move(items_[i]));
- items_[i].~T();
- }
- }
- }
- WP2Free(items_);
- items_ = new_items;
- capacity_ = new_cap;
+ move_items(new_cap, new_items);
}
return true;
}
// Reallocates less memory so that only the existing items can fit.
- bool shrink_to_fit() {
+ WP2_NO_DISCARD bool shrink_to_fit() {
if (capacity_ == num_items_) return true;
if (num_items_ == 0) {
WP2Free(items_);
@@ -122,12 +108,16 @@
// Constructs a new item by copy ctor. May reallocate if 'resize_if_needed'.
WP2_NO_DISCARD bool push_back(const T& value, bool resize_if_needed = true) {
- if (num_items_ >= capacity_ &&
- (!resize_if_needed ||
- !reserve(Internal::NextCapacity(num_items_ + 1)))) {
- return false;
+ if (num_items_ < capacity_) {
+ new (&items_[num_items_]) T(value);
+ } else {
+ if (!resize_if_needed) return false;
+ const size_t new_cap = Internal::NextCapacity(num_items_ + 1);
+ T* const new_items = (T*)WP2Malloc(new_cap, sizeof(T));
+ if (new_items == nullptr) return false;
+ new (&new_items[num_items_]) T(value);
+ move_items(new_cap, new_items);
}
- new (&items_[num_items_]) T(value);
++num_items_;
return true;
}
@@ -179,10 +169,17 @@
}
// Appends 'size' elements by copy. Returns false on error.
- bool append(const T data[], size_t size) {
+ WP2_NO_DISCARD bool append(const T data[], size_t size) {
if (size == 0) return true;
- if (!reserve(Internal::NextCapacity(num_items_ + size))) return false;
- std::copy(data, data + size, &items_[num_items_]);
+ const size_t new_cap = Internal::NextCapacity(num_items_ + size);
+ if (new_cap <= capacity_) {
+ std::copy(data, data + size, &items_[num_items_]);
+ } else {
+ T* const new_items = (T*)WP2Malloc(new_cap, sizeof(T));
+ if (new_items == nullptr) return false;
+ std::copy(data, data + size, &new_items[num_items_]);
+ move_items(new_cap, new_items);
+ }
num_items_ += size;
return true;
}
@@ -223,7 +220,8 @@
swap(capacity_, b.capacity_);
swap(num_items_, b.num_items_);
}
- bool copy_from(const VectorBase& src) {
+ WP2_NO_DISCARD bool copy_from(const VectorBase& src) {
+ if (src.items_ == items_) return true;
clear();
if (!reserve(src.capacity())) return false;
for (const auto& elmt : src) {
@@ -236,6 +234,27 @@
T* items_ = nullptr;
size_t capacity_ = 0;
size_t num_items_ = 0;
+
+ private:
+ // Moves data from items_ into new_items, and have new_items be the new
+ // items_.
+ void move_items(size_t new_cap, T* new_items) {
+ if (num_items_ > 0) {
+ if (std::is_trivially_copyable<T>::value &&
+ std::is_trivially_destructible<T>::value) {
+ std::memcpy((void*)new_items, (const void*)items_,
+ num_items_ * sizeof(T));
+ } else {
+ for (size_t i = 0; i < num_items_; ++i) {
+ new (&new_items[i]) T(std::move(items_[i]));
+ items_[i].~T();
+ }
+ }
+ }
+ WP2Free(items_);
+ items_ = new_items;
+ capacity_ = new_cap;
+ }
};
} // namespace Internal
diff --git a/tests/test_vector.cc b/tests/test_vector.cc
index c2e2015..c40141b 100644
--- a/tests/test_vector.cc
+++ b/tests/test_vector.cc
@@ -26,6 +26,7 @@
class Foo {
public:
Foo(int x = kMagic) : x_(x) {} // NOLINT (explicitly no 'explicit')
+ ~Foo() { x_ = -1; }
operator int() const { return x_; } // NOLINT (explicitly no 'explicit')
private:
@@ -68,38 +69,71 @@
DoTestCtor(0.);
}
-template<typename T> void DoTestPushBack(const T& vector_type) {
- (void)vector_type;
+template <typename VectorType>
+void DoTestPushBack() {
+ // Avoid compilers complaining about comparison of integer expressions of
+ // different signedness.
+ const auto magic = static_cast<typename VectorType::value_type>(kMagic);
+ const auto magic2 = static_cast<typename VectorType::value_type>(kMagic2);
- T v;
+ VectorType v;
EXPECT_TRUE(v.reserve(8));
EXPECT_EQ(v.size(), 0u);
EXPECT_EQ(v.capacity(), 8u);
EXPECT_TRUE(v.push_back(kMagic));
EXPECT_EQ(v.size(), 1u);
- EXPECT_EQ((int)v[0], kMagic);
+ EXPECT_EQ(v[0], magic);
EXPECT_TRUE(v.push_back(kMagic2));
EXPECT_EQ(v.size(), 2u);
- EXPECT_EQ((int)v[0], kMagic);
- EXPECT_EQ((int)v[1], kMagic2);
+ EXPECT_EQ(v[0], magic);
+ EXPECT_EQ(v[1], magic2);
- EXPECT_TRUE(v.resize(1)); // down-size back to 1
+ EXPECT_TRUE(v.resize(1)); // down-size back to 1
EXPECT_EQ(v.size(), 1u);
- EXPECT_EQ((int)v[0], kMagic);
+ EXPECT_EQ(v[0], magic);
- v.resize_no_check(7); // up-size quietly
+ v.resize_no_check(7); // up-size quietly
EXPECT_EQ(v.size(), 7u);
- EXPECT_EQ((int)v[1], kMagic2);
+
+ v.clear();
+ EXPECT_TRUE(v.push_back(kMagic2));
+ for (int i = 0; i < 100; ++i) {
+ EXPECT_TRUE(v.push_back(v[0]));
+ EXPECT_EQ(v[i + 1], magic2);
+ }
+
+ v.clear();
+ EXPECT_TRUE(v.shrink_to_fit());
+ EXPECT_TRUE(v.push_back(kMagic));
+ EXPECT_TRUE(v.push_back(kMagic2));
+ for (int i = 0; i < 100; ++i) {
+ EXPECT_TRUE(v.append(&v[0], 2));
+ EXPECT_EQ(v[v.size() - 2], v[0]);
+ EXPECT_EQ(v[v.size() - 1], v[1]);
+ }
+
+ v.clear();
+ EXPECT_TRUE(v.shrink_to_fit());
+ for (int i = 0; i < 100; ++i) {
+ EXPECT_TRUE(v.push_back(kMagic));
+ EXPECT_TRUE(v.push_back(kMagic2));
+ }
+ const size_t size = v.size();
+ EXPECT_TRUE(v.copy_from(v));
+ for (size_t i = 0; i < size; i += 2) {
+ EXPECT_EQ(v[i], magic);
+ EXPECT_EQ(v[i + 1], magic2);
+ }
}
TEST(VectorTest, PushBack) {
- DoTestPushBack(Vector_u8());
- DoTestPushBack(Vector_s16());
- DoTestPushBack(Vector_u32());
- DoTestPushBack(VectorNoCtor<float>());
- DoTestPushBack(Vector<Foo>());
+ DoTestPushBack<Vector_u8>();
+ DoTestPushBack<Vector_s16>();
+ DoTestPushBack<Vector_u32>();
+ DoTestPushBack<VectorNoCtor<float>>();
+ DoTestPushBack<Vector<Foo>>();
}
// Copy constructor and assignment are deleted, but move constructor and
