Add a range constructor to vecsim_stl::vector - #1008
Open
dor-forer wants to merge 1 commit into
Open
Conversation
vecsim_stl::vector could only be built empty, with a size, or with a size and a fill value, so building one from an existing range meant constructing it empty and then copying elements in by hand. Add an iterator-pair constructor that forwards to the underlying std::vector, keeping the allocator as the trailing argument like the other constructors. It is constrained to std::input_iterator (the same idiom std::vector uses) so that calls such as vector<size_t>(10, 5, alloc) keep resolving to the (count, value) constructor instead of being deduced as a range of ints. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1008 +/- ##
=======================================
Coverage 97.15% 97.15%
=======================================
Files 141 141
Lines 8328 8330 +2
=======================================
+ Hits 8091 8093 +2
Misses 237 237 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
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.
Describe the changes in the pull request
vecsim_stl::vectorexposes only three constructors: empty, sized, and sized-with-fill-value. Building one from an existing range therefore means constructing it empty and copying the elements in by hand.This adds the missing iterator-pair constructor, forwarding to the underlying
std::vectorand keeping the allocator as the trailing argument like the existing constructors:<iterator>is now included explicitly rather than relied upon transitively.Why the
std::input_iteratorconstraint: for a call likevecsim_stl::vector<size_t> v(10, 5, alloc)an unconstrained template would win overload resolution against the(count, value, alloc)constructor, sinceIter = intis an exact match for both arguments while the fill constructor needs aninttosize_tconversion, and conversion-sequence ranking is applied before the prefer-non-template tiebreak. In practice libstdc++ recovers (its own range constructor is SFINAE-guarded, so the forwarded(int, int, alloc)call lands back on its fill constructor), but relying on that is fragile and produces confusing diagnostics for genuinely bad calls. Constraining onstd::input_iteratoris the same idiomstd::vectoritself uses, keeps resolution correct at our own signature, and is free. No behaviour change for any existing call site.Which issues this PR fixes
None. No MOD ticket: this came out of noticing the missing constructor. Happy to attach one if the convention requires it.
Main objects this PR modified
vecsim_stl::vector(src/VecSim/utils/vecsim_stl.h) - added the range constructor.AllocatorTest(tests/unit/test_allocator.cpp) - newtest_vector_range_constructor.Mark if applicable
Neither box applies: the addition is internal C++ (not the
extern "C"API invec_sim.h), purely additive, and touches no serialized state.Testing
New
AllocatorTest.test_vector_range_constructorcovers:VecSimAllocatorin a single allocation of the exact size, and released on destructionvecsim_stl::vectorstd::listrange (non-contiguous iterators, size not known upfront)vector<size_t>(10, 5, alloc)still resolves to the fill constructorVerified on an Intel Xeon Platinum 8375C box (gcc 13.3, AVX512, SVS enabled):
make buildclean, zero errorsmake unit_test: 2607/2607 passed (the 8 reported as skipped are the pre-existing SVS quantization / dynamic-info-iterator cases, unrelated to this change)ctest -R Allocator8/8 passed, including the new casemake check-formatcleanNote on scope
Nothing in
src/calls the new constructor yet. The nearest existing use ishnsw.h:915inmutuallyConnectNewElement, where a freshcandidatesListis immediately filled byinsert(end(), top_candidates.begin(), top_candidates.end()); converting it is a one-line readability change with identical allocation behaviour, and I left it out of this PR to keep a hot path out of scope. The first real consumer is likely the SQ8-with-norm path, whoseQuantPreprocessorconstructor already takes aconst vecsim_stl::vector<float> &mean_vecthat nosrc/caller builds yet.🤖 Generated with Claude Code