Skip to content

[ADT] Add C++17-style insert_or_assign for DenseMap #94151

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

Merged
merged 7 commits into from
Jun 5, 2024
Merged

[ADT] Add C++17-style insert_or_assign for DenseMap #94151

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1724a23
implement insert_or_assign
c8ef Jun 2, 2024
a13126c
test the const ref api
c8ef Jun 3, 2024
9142324
add a subclass to avoid default construct
c8ef Jun 3, 2024
1271ba9
refactor MoveOnly
c8ef Jun 4, 2024
e40b110
Merge branch 'main' into adt
c8ef Jun 5, 2024
6873996
Merge branch 'main' into adt
c8ef Jun 5, 2024
2b0fea1
reset counter for each fragment
c8ef Jun 5, 2024
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
16 changes: 16 additions & 0 deletions llvm/include/llvm/ADT/DenseMap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,22 @@ class DenseMapBase : public DebugEpochBase {
insert(*I);
}

template <typename V>
std::pair<iterator, bool> insert_or_assign(const KeyT &Key, V &&Val) {
auto Ret = try_emplace(Key, std::forward<V>(Val));
if (!Ret.second)
Ret.first->second = std::forward<V>(Val);
return Ret;
}

template <typename V>
std::pair<iterator, bool> insert_or_assign(KeyT &&Key, V &&Val) {
auto Ret = try_emplace(std::move(Key), std::forward<V>(Val));
if (!Ret.second)
Ret.first->second = std::forward<V>(Val);
return Ret;
}

/// Returns the value associated to the key in the map if it exists. If it
/// does not exist, emplace a default value for the key and returns a
/// reference to the newly created value.
Expand Down
28 changes: 28 additions & 0 deletions llvm/unittests/ADT/DenseMapTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -499,6 +499,34 @@ TEST(DenseMapCustomTest, ReserveTest) {
}
}

TEST(DenseMapCustomTest, InsertOrAssignTest) {
DenseMap<int, CountCopyAndMove> Map;
CountCopyAndMove::Copy = 0;
CountCopyAndMove::Move = 0;

CountCopyAndMove val1;
auto try0 = Map.insert_or_assign(0, val1);
EXPECT_TRUE(try0.second);
EXPECT_EQ(0, CountCopyAndMove::Move);
EXPECT_EQ(1, CountCopyAndMove::Copy);

auto try1 = Map.insert_or_assign(0, val1);
EXPECT_FALSE(try1.second);
EXPECT_EQ(0, CountCopyAndMove::Move);
EXPECT_EQ(2, CountCopyAndMove::Copy);

CountCopyAndMove val2;
auto try2 = Map.insert_or_assign(2, val2);
EXPECT_TRUE(try2.second);
EXPECT_EQ(0, CountCopyAndMove::Move);
EXPECT_EQ(3, CountCopyAndMove::Copy);

auto try3 = Map.insert_or_assign(2, std::move(val2));
EXPECT_FALSE(try3.second);
EXPECT_EQ(1, CountCopyAndMove::Move);
EXPECT_EQ(3, CountCopyAndMove::Copy);
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

This test coverage only covers the rvalue ref Key overload, yeah? Be good to have similar coverage for the const ref overload too.

Also, some of the value/point of insert_or_assign is to directly construct the value where possible, and otherwise fallback to assignment. But this test doesn't differentiate between whether the object was default constructed then assigned to, or directly copy/move constructed. To get that more precise test coverage it might be necessary to add more detailed tracking to the CountCopyAndMove helper - but I think we might have a more reusable/generic/detailed type shared across more of these ADT unit tests that could be used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test coverage only covers the rvalue ref Key overload, yeah? Be good to have similar coverage for the const ref overload too.

I have updated the test case to test both APIs.

Also, some of the value/point of insert_or_assign is to directly construct the value where possible, and otherwise fallback to assignment. But this test doesn't differentiate between whether the object was default constructed then assigned to, or directly copy/move constructed. To get that more precise test coverage it might be necessary to add more detailed tracking to the CountCopyAndMove helper - but I think we might have a more reusable/generic/detailed type shared across more of these ADT unit tests that could be used?

I'm not completely sure how to test it as it appears to require integration with the DenseMap internal structure. Could you provide some insight on this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be enough to drop the default constructor on CountCopyAndMove (or add a variant of it that does).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it would be enough to drop the default constructor on CountCopyAndMove (or add a variant of it that does).

Simply removing the default constructor would require modifying many other tests, so I opted to add a subclass to address the issue.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, we have MoveOnly in llvm/unittests/ADT/MoveOnly, which does differentiate counting between the ctor and assignment operator - but doesn't provide copy operations/counting those. Something similar could be added.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The updated test doesn't look like it differentiates between ctor and assignment, though? I guess by virtue of it not being default constructible it necessarily can't be construct+assign in the cases of new values? That's a bit subtle, and I'd have thought testing that only copy/move construction happened on new insertion, and only copy/move assignment happened when insert_or_assigning over an existing value would be more clear about the expectations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The updated test doesn't look like it differentiates between ctor and assignment, though? I guess by virtue of it not being default constructible it necessarily can't be construct+assign in the cases of new values? That's a bit subtle, and I'd have thought testing that only copy/move construction happened on new insertion, and only copy/move assignment happened when insert_or_assigning over an existing value would be more clear about the expectations?

Thank you for your suggestions! I have refactored the MoveOnly class, added statistics regarding the copy constructor and assignment, and tested the insert_or_assign API using this information.

// Make sure DenseMap works with StringRef keys.
TEST(DenseMapCustomTest, StringRefTest) {
DenseMap<StringRef, int> M;
Expand Down
Loading