[llvm] [ADT] Add a missing call to a unique_function destructor after move (PR #98747)

Dmitry Yanovsky via llvm-commits llvm-commits at lists.llvm.org
Tue Jul 16 02:05:55 PDT 2024


================
@@ -310,4 +310,25 @@ class Incomplete {};
 Incomplete incompleteFunction() { return {}; }
 const Incomplete incompleteFunctionConst() { return {}; }
 
+// Check that the moved-from captured state is properly destroyed during
+// move construction/assignment.
+TEST(UniqueFunctionTest, MovedFromStateIsDestroyedCorrectly) {
+  static int NumOfMovesCalled = 0;
+  static int NumOfDestructorsCalled = 0;
+  struct State {
+    State() = default;
+    State(const State &) = delete;
+    State(State &&) { ++NumOfMovesCalled; }
+    State &operator=(const State &) = delete;
+    State &operator=(State &&Rhs) { return *new (this) State{std::move(Rhs)}; }
+    ~State() { ++NumOfDestructorsCalled; }
+  };
+  {
+    unique_function<void()> CapturingFunction{[state = State{}] {}};
+    unique_function<void()> CapturingFunctionMoved{
+        std::move(CapturingFunction)};
+  }
+  EXPECT_EQ(NumOfDestructorsCalled, 1 + NumOfMovesCalled);
----------------
kerambyte wrote:

Sounds good to me, refactored the test to use that helper. Had to upgrade it a bit to include default constructions as well.

While testing I think I might have actually found another bug (which can also explain why this destructor issue didn't come up before). There is this check in the code:

```
if (sizeof(CallableT) > InlineStorageSize ||
    alignof(CallableT) > alignof(decltype(StorageUnion.InlineStorage))) {
    IsInlineStorage = false;
    // ...
}
```

Where `InlineStorage` is defined as:

```
alignas(void *) mutable std::byte InlineStorage[InlineStorageSize];
```

I believe this expression `alignof(decltype(StorageUnion.InlineStorage))` actually returns 1 and so we always skip the inline storage code-path (CompilerExplorer [seems to agree](https://godbolt.org/z/oEPsfTqWK)).

I've fixed that by using an explicit `InlineStorageAlign` variable but let me know if you want me to split this PR into two.

https://github.com/llvm/llvm-project/pull/98747


More information about the llvm-commits mailing list