Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make length type generic #256

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

JakkuSakura
Copy link
Contributor

It's really a lot of work being done
This PR resolves #247, supersedes #248
However, some things are broken:
type inference won't work for ArrayVec::from([0; N]) like before
some functions like len() are no longer const, unless nightly is enabled

This PR is expected to be merged after #255

@JakkuSakura JakkuSakura force-pushed the generic-length-type-over-new-layout branch from 812ca47 to 4fb1f1b Compare December 29, 2023 13:59
@JakkuSakura JakkuSakura force-pushed the generic-length-type-over-new-layout branch from 4fb1f1b to eefee31 Compare December 29, 2023 14:00
@GnomedDev
Copy link

I'm really interested in this, but it seems like there are conflicts that need to be resolved @JakkuSakura?

@JakkuSakura
Copy link
Contributor Author

Hi, while it's easy to resolve the conflicts, the potentially breaking semantics should be carefully evaluated by the author, before merging

@GnomedDev
Copy link

Okay, I've opened a PR on your repo with some fun type system hacks that fix the inference for From<[T; CAP]> in a lot of cases, and also makes the default use the proper type instead of u32 in a lot of cases. If/When trait associated type defaults are a thing, this will become basically perfect, but as-is it needs users to provide LenT if the CAP value isn't in the predefined list of CAP => LenT.

@JakkuSakura
Copy link
Contributor Author

It's almost perfect

        let string = ArrayString::<11>::from_byte_string(b"hello world").unwrap();

the only tests that failed was ArrayString without <11>. it's related to the way rust handles inferred type with default type generics.

@honzasp
Copy link
Contributor

honzasp commented Aug 8, 2024

Hi folks, I'm also really interested in this feature. Is there anything I could help with to move this forward?

@bluss
Copy link
Owner

bluss commented Aug 17, 2024

Well, as you know it's a change where you add something you really want, but we pay with losing some features (type inference, predictability (dependency on capacity), const fn).

For any maintainer it's much easier to accept a PR if it's a win-win or if there are only upsides. Differentiated length type was something we had but lost when moving from ad-hoc to const generics, and I think it was worth it.

@honzasp
Copy link
Contributor

honzasp commented Aug 17, 2024

The issue with type inference of the LenType argument could be solved by simply always defaulting to u32: that should maintain full backward compatilbility, and users would still be able to opt into a narrower type where it makes sense.


However, the problem with some functions such as len() losing their const-ness is harder. Until we get const fn in traits, the only solution that I can think of is to define three separate types, ArrayVec, ArrayVec16 and ArrayVec8, using macros.

The advantage of this solution is that it maintains full backward compatibility, the disadvantage is that it makes the implementation more complicated and compilation a bit slower. @bluss, what do you think, would you consider such a PR?

@bluss
Copy link
Owner

bluss commented Aug 17, 2024

I would say multiple types is the better path. Or a new type that's always instantiated with a particular capacity type like ArrayVec<T, CAP> = ArrayVecX<T, U32Capacity, CAP>

With that said, I thought there should be some clever way to preserve const fn len.

There is an existing mechanism for reducing code duplication - find ArrayVecImpl in the code. The idea is that it provides implementation sharing of different ArrayVec flavors (ArrayVec vs ArrayVecCopy). This needs to be used fully (or find an equivalent but better way to do it) to avoid duplicating the hard code (read: the unsafe code).

I'm not concerned about compilation time that much. ArrayVec is generic in the element type, and that means the user doesn't pay much for compilation time unless they instantiate a particular array vec type. No codegen runs for generic items, only monomorphic items.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can I change LenUint to u8?
4 participants