Add allocation tests by pedrodesu · Pull Request #745 · servo/rust-smallvec

pedrodesu · 2026-10-07T09:10:27Z

This has quite a bit of stuff so I'm open to comments. Closes #669. From what I see, everything passes. I believe the current tests are a good enough baseline but I'm open to more ideas. While working on this I also reached the conclusion that we should probably have better, proper doc comments (perhaps mirroring std) for our functions. I think that would be a great idea.
In this PR I also took the liberty to convert from_raw_parts and into_raw_parts into from_parts and into_parts respectively. This seemed like a necessity to properly implement the new tests. The former work just like the latter but they use NonNull instead and I believe it's a correct assessment that they're obsolete functions only kept for compatibility with earlier rust versions, superseded by their NonNull counterparts. As such, I don't think they're a necessity in this crate and we would be better off keeping and keeping only the latter. Let me know what you think about this. In the same fashion, I also implemented the similar, allocator explicit counterparts from_parts_in and into_parts_with_alloc. This also led me to the conclusion that our transition from Global-only functions to more generic functions is still somewhat incomplete: The impl<T, const N: usize> SmallVec<T, N, Global> should definitely be much smaller than it is currently; I think it should only keep convenience functions that are probably just wrappers for also implemented, more generic versions of the same functions that work for any allocator (such as SmallVec::new vs SmallVec::new_in).

…ests and implementation of *_parts, also for the allocator counterparts. replaced *_raw_parts

pedrodesu · 2026-10-07T09:17:52Z

I also had to fix cargo fmt for encase::rts_array::impl_rts_array. No idea why this failed in the first place, as I didn't change it. I'm guessing this might have happened because of a prior PR that passed.

pedrodesu · 2026-10-07T09:24:26Z

As previously discussed, the new tests only execute if we have access to the allocator API.

alejandro-vaz · 2026-10-07T11:28:54Z

brother how is this +800 LOC??

pedrodesu · 2026-10-07T11:31:58Z

It creates two dummy allocator structures and features a comprehensive suite of multiple cases and circumstances where the allocations must be kept in check. I think this is what we wanted, no? This is partly why the PR took so long. A few hundred wouldn't cut it for an in-depth test case. :)

alejandro-vaz · 2026-10-07T11:33:09Z

yeah yeah, I was just expecting something a bit shorter

I'll review it tomorrow

pedrodesu · 2026-10-07T11:33:59Z

Also I just noticed, the default feature change on the Cargo.toml was an incorrect leftover from my local development. I'll reverse it when I get back home.

pedrodesu · 2026-10-07T11:34:58Z

yeah yeah, I was just expecting something a bit shorter

I'll review it today

Take your time. As I said, open to criticisms, I'm aware it's quite a big PR.

Signed-off-by: Pedro Nobre <me.pedro.nobre@pm.me>

…location.rs` Signed-off-by: Pedro Nobre <me.pedro.nobre@pm.me>

alejandro-vaz · 2026-10-08T12:20:54Z

            size_of
        },
        ptr::{
+            self,


can we not import the module as such

You'd prefer to have another use expressly for use core::ptr? Why? Or did you just prefer to call core::ptr::read directly on the call site?

just call <*mut T>::read or import core::ptr::read directly

I feel like using core::ptr::read is the idiomatic solution (and we get free pointer coercion) but by importing core::ptr::read the call site becomes simply read(). This means exposing a vague, generic function with no further context. This is why I liked ptr::read - it keeps the intent clear but it's smaller and abstracts whether we're calling from core or std (which is irrelevant at the call site). I move we simply use core::ptr::read directly on the call site. Seems reasonable enough.

simply call the .read() method and that's it why complicate it further

&me.allocator.read() doesn't exist. &me.allocator is not a pointer. You need to either coerce it into a pointer or call ptr::read, which does that for you.

the idiomatic way is then (&raw const me.allocator).read()

ptr::read was definitely made for these such cases where you need to coerce a reference into a pointer first. It's the most ergonomic solution, with least work. As such, I feel like it's definitely more idiomatic than doing that coercion yourself manually and then calling read on the created pointer, when ptr::read does both for you cleanly. But I agree to settle with &raw const.read(), this is mostly a non-issue.

alejandro-vaz · 2026-10-08T12:23:45Z

honestly I'd remove most comments for now, or at the very least reduce them

I may be the only dev who says it, but I'm not reading that. I just don't. zero percent chance.

in rc we will figure out documentation, for now we can just make it as minimal as possible

I don't agree with that at all, I feel like std puts a very good standard on doc comments and following such a standard is a great practice in regards to ensuring that the function contract is very well explained and detailed. For instance, a lot of the functions had constraints that were not listed, but that needed to be ensured when calling them or you'd get UB. This isn't something small. One can say such constraints were expected, but guesswork is probably not ideal, else why comment on the functions at all? Should we leave to the developer to understand on runtime what causes UB and what does not? Or how the function actually works?
With that in mind, it's your call. Let me know what you'd prefer me to do with the comments.

alejandro-vaz · 2026-10-08T12:24:44Z

    /// }
    /// ```
    #[inline]
-    pub unsafe fn from_raw_parts(


I wouldn't remove this function

we should probably have both the *_raw_parts and the *_parts variants

I can definitely keep both. With that said, is there a specific, strong reason why you'd wanna keep *_raw_parts? As I explained when creating the PR, these function seem largely unnecessary, when they're quite literally just *mut wrappers for the NonNull counterparts. Dated, for when NonNull wasn't the norm. Calling them with null is UB, calling *_parts with null isn't possible.

alejandro-vaz · 2026-10-08T12:26:08Z

    /// ```
    #[inline]
-    pub fn into_raw_parts(self) -> (*mut T, usize, usize) {
+    pub fn into_parts_with_alloc(self) -> (NonNull<T>, usize, usize, A) {


I'm not fully sure of this but I think we should not have an into_parts_with_alloc function

I think the normal into_parts / into_raw_parts should take and return the allocator

if you think about it, the only reason Vec doesn't have this behavior is backwards compatibility, not because it isn't correct

That's very fair. I kept it like this because it's what the std does. This, however, means that we don't have a "default", simpler into_(raw)_parts that doesn't return an allocator. That might make sense when the developer doesn't need to be exposed to and doesn't want to see allocators at all, which is definitely the majority of the cases as of now. Namely, when simply using Global. Essentially, it's the same argument as Vec::new vs Vec::new_in. Having a case that omits the allocator when not needed and assuming the default. Let me know what you think.

alejandro-vaz · 2026-10-08T12:28:57Z

-fn into_raw_parts_inline() {
+fn into_parts_inline() {
    let v: SmallVec<i32, 10> = SmallVec::from([1, 2, 3]);
-    v.into_raw_parts();
+    v.into_parts();
 }

 #[test]
-fn into_raw_parts_heap() {
+fn into_parts_heap() {
    let v: SmallVec<i32, 1> = SmallVec::from([1, 2, 3]);
-    let (ptr, length, capacity) = v.into_raw_parts();


keep these tests as well, see above

alejandro-vaz · 2026-10-08T12:31:11Z

brother this is extremely complicated and verbose

this could be made much simpler

conceptually what we need is an allocator that delegates to the global allocator but that tracks stuff on the way, that's it

no refcell, not that many implementations, and then we simply compare numbers

You just defined what we're doing here though?
TestAlloc delegates to the global allocator (technically System. Global = System as long as the developer doesn't change it. In the case of tests, it of course isn't changed. However I do indeed see now that Global is probably better, as that means not relying on std) but tracks stuff (alloc, dealloc, grow, shrink, allocated_bytes and aligns, exactly all that we need to track to ensure that the calls work as expected) on the way.

no refcell

Indeed RefCell isn't necessary here, but Cell at least will be needed for the inner mutability under &self. When I first wrote this I thought Cell wasn't enough, but indeed it is. Not that it matters too much however, these are tests.

and then we simply compare numbers

That is exactly what these tests do.

not that many implementations

I can certainly shrink the amount of implementations, but 1) I don't understand why we would want to cover less cases where allocation may happen incorrectly and 2) I thought you specifically asked for an extensive test suite to validate allocations?

I don't think I understood exactly what you expected from this or what is wrong with these tests. It seems like all of these are valuable. If not, why test allocations in the first place? And how, if not like this?

…stop depending on `std`. switch `TestAlloc`'s `RefCell` for `Cell`

Signed-off-by: Pedro Nobre <me.pedro.nobre@pm.me>

pedrodesu added 4 commits October 7, 2026 10:03

tests: write allocation tests; feat: small refactors needed for the t…

a75eb23

…ests and implementation of *_parts, also for the allocator counterparts. replaced *_raw_parts

fix: small name fix

56a755c

fix: small name fix, missed the tests

93822f7

practices: fix cargo fmt for encase impl

8de6553

alejandro-vaz self-requested a review October 7, 2026 11:22

alejandro-vaz assigned pedrodesu Oct 7, 2026

pedrodesu added 2 commits October 7, 2026 13:05

fix: remove default feature 'encase' again

2851fc1

Signed-off-by: Pedro Nobre <me.pedro.nobre@pm.me>

test: removed redundant allocator-api2 feature requirement from `al…

a956f06

…location.rs` Signed-off-by: Pedro Nobre <me.pedro.nobre@pm.me>

alejandro-vaz requested changes Oct 8, 2026

View reviewed changes

pedrodesu and others added 2 commits October 8, 2026 17:16

test: on allocation, swap std's System for alloc's Global. …

1e7b2a2

…stop depending on `std`. switch `TestAlloc`'s `RefCell` for `Cell`

fix: revert default feature

c68c16a

Signed-off-by: Pedro Nobre <me.pedro.nobre@pm.me>

Conversation

pedrodesu commented Oct 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pedrodesu commented Oct 7, 2026

Uh oh!

pedrodesu commented Oct 7, 2026

Uh oh!

alejandro-vaz commented Oct 7, 2026

Uh oh!

pedrodesu commented Oct 7, 2026

Uh oh!

alejandro-vaz commented Oct 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pedrodesu commented Oct 7, 2026

Uh oh!

pedrodesu commented Oct 7, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pedrodesu Oct 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pedrodesu Oct 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pedrodesu Oct 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pedrodesu Oct 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pedrodesu commented Oct 7, 2026 •

edited

Loading

alejandro-vaz commented Oct 7, 2026 •

edited

Loading

pedrodesu Oct 8, 2026 •

edited

Loading

pedrodesu Oct 8, 2026 •

edited

Loading

pedrodesu Oct 8, 2026 •

edited

Loading

pedrodesu Oct 8, 2026 •

edited

Loading