[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