Repository navigation
Apply claude patches - #422
Open
charles-lunarg wants to merge 11 commits into
Open
charles-lunarg wants to merge 11 commits into
charles-lunarg wants to merge 11 commits into
Conversation
The header used std::numeric_limits without including <limits>, so a translation unit that included it first failed to compile. It also relied on transitive includes for uint32_t/uint8_t, std::min, std::initializer_list and the <type_traits> helpers. Include the header first in tests/small_containers.cpp so this stays checked. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
emplace_back constructed the new element with value_type(args...), so every argument was passed as an lvalue. emplace_back(std::move(x)) copied instead of moving. Move-only element types such as std::unique_ptr failed to compile, although small::vector documents MoveConstructible as its requirement. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
When the vector was full, emplace_back reallocated first and then built the new element from args. If args referred to an element of the same vector, e.g. v.emplace_back(v[0]), that element had already been moved out and destroyed. Growing out of the small store copied a moved-from value. Growing a large store read freed memory (ASan heap-use-after-free). Build the new element before reallocating when the vector is full. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Once the small store was full, emplace_back called reserve(size_ + 1), which allocates exactly the requested capacity. Every later push reallocated and moved all existing elements, so filling a vector with n elements was O(n^2). 20000 emplace_backs did about 2e8 element moves, compared with 32767 for std::vector. Double the capacity when full, saturating at the size_type maximum. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
resize(count, value) reserved the new capacity before copying value into the new slots. If value referred to an element of the same vector, e.g. v.resize(n, v[0]), reserve() had already moved it out and destroyed it. The new elements were then copied from a moved-from or freed object (ASan heap-use-after-free). Copy value before reallocating. Also build the new elements with a single copy instead of a copy followed by a move. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
shrink_to_fit move-assigned each element into the new store, which is raw storage with no object in it. That is undefined behavior for any non-trivial type. With std::string it crashes inside basic_string::operator=. Use placement new to move-construct the elements instead. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
With exactly kSmallCapacity elements, shrink_to_fit allocated a heap store of capacity N instead of moving back into the small store. That wastes an allocation and leaves large_store_ set with capacity_ == N. The move constructor and move assignment then take the large store path and assert other.capacity_ > kSmallCapacity in MoveLargeStore. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The rvalue overload PushBackFrom(Container &&) takes a forwarding reference, so for a non-const lvalue it is a better match than PushBackFrom(const Container &). v.PushBackFrom(src) then moved every element out of src, leaving it full of moved-from values. Restrict that overload to rvalues. Also give it the same overflow asserts as the const overload. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The move constructor and move assignment weren't noexcept, so std::vector<small::vector<T, N>> and other std containers copied their elements when reallocating instead of moving them. For a move-only T that doesn't compile. Both operations only move elements or steal the large store, so make them conditionally noexcept on T's move operations. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
erase() and clear() only cleared the slot's allocated flag for elements in the inline storage. The element itself stayed alive until the slot was reused or the container was destroyed. An erased std::shared_ptr value kept its object alive: use_count stayed at 2 after erase(key) and after clear(). Reset the slot to a default value when it is freed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
|
||
| #pragma once | ||
|
|
||
| #include <algorithm> |
Contributor
There was a problem hiding this comment.
the amount of new includes for this utility header is not a bit scary
Collaborator
Author
There was a problem hiding this comment.
Yes, but thats because we use a ton of the C++ standard library.
std::min comes from <algorithm>
size_t from <cstddef>
uint32_t from <cstdint>
initializer_list from I think you can guess
numeric_limits from <limits>
a TON of type_traits
and the std::forward/std::move/std::pair utilities.
So these includes should of been here the whole time.
Contributor
There was a problem hiding this comment.
we need to include ALL of algorithm for min... we using it as a template actually... that one always amazed me we can't write that ourselves
jeremyg-lunarg
approved these changes
Oct 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Spencer asked Claude to review VUL, here is what it found.
I reviewed the small containers changes, they seem good. The range map & concurrent unordered_map changes were much harder to validate (as I am out of my depth). I have not added them in order to limit the scope of this PR. Well, save for one commit which I was able to understand.
I did have to fix a few signed/unsigned issues that made clang unhappy. GCC didn't mind, but its a bit concerning that I had to specify an unsigned int when indexing a small_vector, and the compiler couldn't figure out that the literal
64was fine to turn into an unsigned type when constructing a string.