Make RawSmallVec (and its fields) public only under feature = "internals". (v2)

[zachs18]

It doesn't have any public API other than auto traits and blanket impls, and no other public APIs take or return it, so it probably doesn't need to be public.

(Alternately, it's fields could be maked pub so external users could use it for their own purposes, but it would probably be better for them to just define their own version.)

[zachs18]

I just saw #245 which may make the "Alternately" the desired route.

[alejandro-vaz]

RawVec is not public in the std API

I do get the motivation behind it: other std types like VecDeque use RawVec internally, so why wouldn't other crates by relying on smallvec??

what about having a feature flag that makes it public?? e.g. "rawsmallvec" (or even "internals" if we are going to re-export other types as well)

so by default that feature would be disabled, but could be turned on by other crates to access it

it wouldn't be a great idea to export it in the public api by default because it's not a type that's meant to be used as such by most downstream crates

[alejandro-vaz]

also, sorry for the long delay

[alejandro-vaz]

see above

[zachs18]

We could add an cargo feature to make RawSmallVec and related APIs public, but that might make it difficult to change them in the future in otherwise-backwards-compatible ways. (e.g. if we wanted to change the heap field of RawSmallVec from (NonNull<T>, usize) to NonNull<[T]> or something).

I've pushed 1861ee4 (after a rebase) which makes RawSmallVec (and its fields) public under the new feature = "internals", and makes it private without that feature enabled. There's nothing else under this feature in this PR.

[zachs18]

(Also changes the doc comment to not claim that there's a niche, since unions (currently) never have niches)

[alejandro-vaz]

We could add an cargo feature to make RawSmallVec and related APIs public, but that might make it difficult to change them in the future in otherwise-backwards-compatible ways. (e.g. if we wanted to change the heap field of RawSmallVec from (NonNull<T>, usize) to NonNull<[T]> or something).

I've pushed 1861ee4 (after a rebase) which makes RawSmallVec (and its fields) public under the new feature = "internals", and makes it private without that feature enabled. There's nothing else under this feature in this PR.

good, thanks

RawSmallVec and SmallVec won't really change their representations ever. the current data structures we have are the most optimal in terms of size, and are very fast with TaggedLen

I literally spent a few minutes this morning looking at those two structs, there's no way I found that scales with size and has that performance

so we can be pretty sure that the union won't change

and we wouldn't be able to switch to something like NonNull<[T]> (a fat pointer) because our TaggedLen has a bit it uses for union variant tracking. it's not true "length"

[alejandro-vaz]

remove this unnecessary module

the way is to leave it as it was, and then add a re-export

so:

#[cfg(feature = "internals")]
pub use RawSmallVec; // self::RawSmallVec

[zachs18]

I tried this, and it doesn't work, for two reasons:

1. you can't pub use an item that isn't pub
2. you can't have two items in the same module with the same name, and a use counts as a separate item even if the name it conflicts with is the thing it is re-exporting.

Details

```rs
struct Foo;
pub use Foo;

error[E0255]: the name `Foo` is defined multiple times
 --> src/lib.rs:3:9
  |
1 | struct Foo;
  | ----------- previous definition of the type `Foo` here
2 |
3 | pub use Foo;
  |         ^^^ `Foo` reimported here
  |
  = note: `Foo` must be defined only once in the type namespace of this module
help: you can use `as` to change the binding name of the import
  |
3 | pub use Foo as OtherFoo;
  |             +++++++++++

error[E0364]: `Foo` is only public within the crate, and cannot be re-exported outside
 --> src/lib.rs:3:9
  |
3 | pub use Foo;
  |         ^^^
  |
note: consider marking `Foo` as `pub` in the imported module
 --> src/lib.rs:3:9
  |
3 | pub use Foo;
  |         ^^^

[alejandro-vaz]

oh yeah this is bad, you're right

so when it's the same file it'd do this stuff

actually do this instead

move the full pub RawSmallVec implementation into a separate module like rawsmallvec.rs and which is simply a mod rawsmallvec:: and then the lib.rs re-exports it (properly)

this is part of #427, that we haven't implemented yet

[zachs18]

we could instead have a macro emit the declaration, like

macro_rules! declare_rsv {
 ($vis:vis) => {
   $vis union Whatever { /* ... */ }
 };
}

#[cfg(feature = "internals")]
declare_rsv!(pub);
#[cfg(not(feature = "internals"))]
declare_rsv!(pub(crate));

[alejandro-vaz]

no need to do so

with

mod rawsmallvec;

#[cfg(feature = "internals")]
pub use rawsmallvec::RawSmallVec;
#[cfg(not(feature = "internals"))] // if this is needed, now it is at least
use rawsmallvec::RawSmallVec;

rust book's rule: don't make a macro out of which doesn't need to be a macro

and we do have to modularize lib.rs because having a 3k file encompassing the whole universe is not very manageable

[zachs18]

Okay, latest push moves mod raw to a file and renames it rawsmallvec (and rearranges the imports a bit).

[zachs18]

we can be pretty sure that the union won't change

👍

and we wouldn't be able to switch to something like NonNull<[T]> (a fat pointer) because our TaggedLen has a bit it uses for union variant tracking. it's not true "length"

The length of the [T] would be the capacity in that case. Perhaps more clearly, it could be NonNull<[MaybeUninit<T>]> (or maybe even ManuallyDrop<Box<[MaybeUninit<T>]>>). In any case, (NonNull<T>, usize) is Fine:tm: for the purposes of this, so it doesn't matter much.

[alejandro-vaz]

we can be pretty sure that the union won't change

👍

and we wouldn't be able to switch to something like NonNull<[T]> (a fat pointer) because our TaggedLen has a bit it uses for union variant tracking. it's not true "length"

The length of the [T] would be the capacity in that case. Perhaps more clearly, it could be NonNull<[MaybeUninit<T>]> (or maybe even ManuallyDrop<Box<[MaybeUninit<T>]>>). In any case, (NonNull<T>, usize) is Fine:tm: for the purposes of this, so it doesn't matter much.

it doesn't sound bad but it'd be a nightmare to implement it and would make things worse probably

I'm not completely sure, I haven't dug very deep into it

I annotated it anyway. NonNull<[MaybeUninit<T>]> where the invariant is that the elements are initialized at 0..len and the slice length is the capacity

[emilio]

Not sure splitting to its own file is really beneficial but...