Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
12 changes: 11 additions & 1 deletion llvm/include/llvm/ADT/STLExtras.h
Original file line number Diff line number Diff line change
Expand Up @@ -2152,7 +2152,17 @@ void append_range(Container &C, Range &&R) {
/// Appends all `Values` to container `C`.
template <typename Container, typename... Args>
void append_values(Container &C, Args &&...Values) {
C.reserve(range_size(C) + sizeof...(Args));
if (size_t InitialSize = range_size(C); InitialSize == 0) {
Copy link
Contributor

@nikic nikic Dec 13, 2025

Choose a reason for hiding this comment

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

Side note: This code should be using llvm::size() and not range_size(). It makes no sense to do a full container scan with std::distance() in this context.

If we have any usages where size() is not available, those should just not attempt to reserve() at all.

Copy link
Member Author

@kuhar kuhar Dec 13, 2025

Choose a reason for hiding this comment

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

I got back to this line of work recently and think we need some fast size function that allows for both random access iterators and containers with .size(), so a combination of std::size (arrays and .size()) and llvm::size (random access only).

Here specifically, I'm not aware of any containers that have .insert() but don't have (fast) .size() (almost every usage is with vector and small vector), so ACK but I'm going to address this separately.

// Only reserve if the container is empty. Reserving on a non-empty
// container may interfere with the exponential growth strategy, if the
// container does not round up the capacity. Consider `append_values` called
// repeatedly in a loop: each call would reserve exactly `size + N`, causing
// the capacity to grow linearly (e.g., 100 -> 105 -> 110 -> ...) instead of
// exponentially (e.g., 100 -> 200 -> ...). Linear growth turns the
// amortized O(1) append into O(n) because every few insertions trigger a
// reallocation and copy of all elements.
C.reserve(InitialSize + sizeof...(Args));
}
// Append all values one by one.
((void)C.insert(C.end(), std::forward<Args>(Values)), ...);
}
Expand Down
35 changes: 35 additions & 0 deletions llvm/unittests/ADT/STLExtrasTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -701,6 +701,41 @@ TEST(STLExtrasTest, AppendValues) {
EXPECT_THAT(Set, UnorderedElementsAre(1, 2, 3));
}

TEST(STLExtrasTest, AppendValuesReserve) {
// A vector wrapper that tracks reserve() calls.
struct TrackedVector : std::vector<int> {
using std::vector<int>::vector;
size_t LastReservedSize = 0;
unsigned ReserveCallCount = 0;

void reserve(size_t N) {
LastReservedSize = N;
++ReserveCallCount;
std::vector<int>::reserve(N);
}
};

// When empty, reserve should be called.
TrackedVector Empty;
append_values(Empty, 1, 2, 3);
EXPECT_EQ(Empty.ReserveCallCount, 1u);
EXPECT_EQ(Empty.LastReservedSize, 3u);
EXPECT_THAT(Empty, ElementsAre(1, 2, 3));

// Appending more values to a now non-empty container should still not
// reserve.
append_values(Empty, 4, 5);
EXPECT_EQ(Empty.ReserveCallCount, 1u);
EXPECT_THAT(Empty, ElementsAre(1, 2, 3, 4, 5));

// When non-empty, reserve should NOT be called to avoid preventing
// exponential growth.
TrackedVector NonEmpty = {1, 2};
append_values(NonEmpty, 3, 4);
EXPECT_EQ(NonEmpty.ReserveCallCount, 0u);
EXPECT_THAT(NonEmpty, ElementsAre(1, 2, 3, 4));
}

TEST(STLExtrasTest, ADLTest) {
some_namespace::some_struct s{{1, 2, 3, 4, 5}, ""};
some_namespace::some_struct s2{{2, 4, 6, 8, 10}, ""};
Expand Down