Reduce bloat from smallvec! in the inline case

[bolshoytoster]

See #418 for details.

[alejandro-vaz]

shouldn't this be instead

($($x:expr),* $(,)?) => // ...

[alejandro-vaz]

though it wouldn't make sense calling smallvec[,], and it actually isn't a valid array, so it should probably be

($($($x:expr),+ $(,)?)?) => // ...

[alejandro-vaz]

and then the () => // ... branch can disappear because the glob pattern of the one with elements it would cover nullable tokenstreams

[alejandro-vaz]

this branch can be removed if completely

[bolshoytoster]

Thanks, I missed that.

[alejandro-vaz]

something similar should be done here, removing @one recursion

[bolshoytoster]

Would that change the behavior of the macro? It currently outputs a SmallVec with the exact capacity for the items passed. If I removed the const N: usize = 0usize $(+ $crate::smallvec_inline!(@one $x))*; and just had it infer N, then it would be identical to the other macro.

Maybe something like

    ($($x:expr),+ $(,)?) => ({
        let buf = [$($x,)*];
        $crate::SmallVec::from_buf_and_len(buf, buf.len())
    });

would work?

[alejandro-vaz]

actually to me it how it is written now seems wrong

smallvec_inline! should basically be the parity of smallvec! but all being inline

so I honestly think it should be

    ($($x:expr),+ $(,)?) => ({
        $crate::SmallVec::from_buf([$($x),*])
        // from_buf::<S>([_; N]) lets S be inferred from the caller
        // so:
        // let s: SmallVec<usize, 4> = smallvec_inline![1, 2]
    });

[alejandro-vaz]

currently it is counting so it doesn't allow to get from context the desired maximum capacity, but that's not what smallvec! does

and they should probably behave equivalently

[bolshoytoster]

Would that be a breaking change? And if we do that, we may as well just remove smallvec_inline! in favor of smallvec!.

Actually, looking at the blame, it looks like smallvec_inline! came from #265 (2021) and might just be obselete now.

It would still be a breaking change to remove it, so maybe it could just forward to smallvec!?

[alejandro-vaz]

smallvec v2 is on alpha so breaking changes aren't an specially big deal if we get the architecture right

I mean, yeah, the breaking change would be that anyone who is using smallvec_inline! couldn't be relying on the macro for inferring the value of N

it doesn't seem intentional to me that they let constant fixed

but it is true that we could just remove the macro entirely then

but what use case does smallvec_inline! have?? basically none. just use smallvec! and annotate or (most of the time) the compiler will infer the value you want

I'd lean now towards removing the smallvec_inline! macro altogether

I'll request a second reviewer later, but I think this is the right direction for such a small macro whose use case is negligible and was meant for a hack 5 years ago

[alejandro-vaz]

remove it (and if it has any uses in the codebase simply replace) and the pr is ready

[bolshoytoster]

There may be a use, actually. smallvec_inline can be used in const, but From<[T, M]> (and therefore smallvec!) isn't const.

[alejandro-vaz]

this discussion will be continued on #432

[alejandro-vaz]

$crate::SmallVec::from([$($($x,)+)?])

should be

$crate::SmallVec::from([$($($x),+)?])

weird rust quirk in which the outside comma means there is no trailing comma

probably doesn't matter much but it matches the branch signature

[bolshoytoster]

I left the comma inside for consistency because the original did it in the heap path. smallvec_inline also has the comma inside. I can change it if you prefer it outside.

[alejandro-vaz]

yeah remove it everywhere you see it

why would anyone want a trailing comma in macro_rules generation everywhere

hahahaha it's so tiny it's laughable but it doesn't make sense

like if it had been intentionally made that way