BLI: Destroy non-trivial types after using remove_if #115914

Guillermo Venegas · 2023-12-07T21:58:45+01:00

Guillermo Venegas commented

2023-12-07 21:58:45 +01:00

blender::Vector::remove_if allows certain elements to be removed based on a predicate,
however std::remove_if only shifts the elements that will not be deleted
at the beginning of the container and then blender::Vector::remove_if only
updates the pointer to the new last element after using std::remove_if.
This works well if blender::Vector is used with trivial types that don't
have a destructor.

Having a blender::Vector<std::unique_ptr> for example,
can generate undefined behavior if the predicate gives true to elements that
are contiguous at the end, if blender::Vector::remove_if only updates the end
pointer in these specific cases, these makes these smart pointers useless because
they will not be freed by themselves.

To prevent that, also destroys the elements being removed.

`blender::Vector::remove_if` allows certain elements to be removed based on a predicate, however `std::remove_if` only shifts the elements that will not be deleted at the beginning of the container and then `blender::Vector::remove_if` only updates the pointer to the new last element after using `std::remove_if`. This works well if `blender::Vector` is used with trivial types that don't have a destructor. Having a `blender::Vector<std::unique_ptr>` for example, can generate undefined behavior if the predicate gives `true` to elements that are contiguous at the end, if `blender::Vector::remove_if` only updates the end pointer in these specific cases, these makes these smart pointers useless because they will not be freed by themselves. To prevent that, also destroys the elements being removed.

Guillermo Venegas added 1 commit 2023-12-07 21:58:54 +01:00

a6b42ee0f1 fixes

Guillermo Venegas added 1 commit 2023-12-07 22:32:36 +01:00

6c8b78d6ab avoid swap for test

Guillermo Venegas added 1 commit 2023-12-07 22:36:51 +01:00

95027944b6 missing change

Guillermo Venegas added 1 commit 2023-12-08 02:41:50 +01:00

5dfc9042e1 fix naming

Iliya Katushenock requested review from Jacques Lucke 2023-12-08 08:06:06 +01:00

Iliya Katushenock added this to the Core Libraries project 2023-12-08 08:06:13 +01:00

Jacques Lucke commented

2023-12-11 12:48:57 +01:00

Thanks for the fix. Not quite sure how I missed that. Can you replace your test with this?

TEST(vector, RemoveIfNonTrivialDestructible)
{
  Vector<Vector<int, 0, GuardedAllocator>> vec;
  for ([[maybe_unused]] const int64_t i : IndexRange(10)) {
    /* This test relies on leak detection to run after tests. */
    vec.append(Vector<int, 0, GuardedAllocator>(100));
  }
  vec.remove_if([&](const auto & /*value*/) { return true; });
  EXPECT_TRUE(vec.is_empty());
}

I prefer this test right now because it has much less boilerplate which can be tricky to get right.
Btw, I also got a compile error because the released_flag parameter shadows a data member with the same name. That probably happens because of the BLI_strict_flags.h include at the top.

Thanks for the fix. Not quite sure how I missed that. Can you replace your test with this? ```cpp TEST(vector, RemoveIfNonTrivialDestructible) { Vector<Vector<int, 0, GuardedAllocator>> vec; for ([[maybe_unused]] const int64_t i : IndexRange(10)) { /* This test relies on leak detection to run after tests. */ vec.append(Vector<int, 0, GuardedAllocator>(100)); } vec.remove_if([&](const auto & /*value*/) { return true; }); EXPECT_TRUE(vec.is_empty()); } ``` I prefer this test right now because it has much less boilerplate which can be tricky to get right. Btw, I also got a compile error because the `released_flag` parameter shadows a data member with the same name. That probably happens because of the `BLI_strict_flags.h` include at the top.

👍 1

Jacques Lucke requested changes 2023-12-11 12:49:24 +01:00

Jacques Lucke left a comment

See comment above.

Guillermo Venegas added 2 commits 2023-12-11 14:30:32 +01:00

c30583af43 simplify test

buildbot/vexp-code-patch-lint Build done. Details

buildbot/vexp-code-patch-linux-x86_64 Build done. Details

buildbot/vexp-code-patch-darwin-x86_64 Build done. Details

buildbot/vexp-code-patch-darwin-arm64 Build done. Details

buildbot/vexp-code-patch-windows-amd64 Build done. Details

buildbot/vexp-code-patch-coordinator Build done. Details