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

Restructure AsVarULE or get rid of it #1078

Closed
Manishearth opened this issue Sep 22, 2021 · 2 comments · Fixed by #1126
Closed

Restructure AsVarULE or get rid of it #1078

Manishearth opened this issue Sep 22, 2021 · 2 comments · Fixed by #1126
Assignees
Labels
blocked A dependency must be resolved before this is actionable C-data-infra Component: provider, datagen, fallback, adapters S-medium Size: Less than a week (larger bug fix or enhancement)
Milestone

Comments

@Manishearth
Copy link
Member

Manishearth commented Sep 22, 2021

Part of #1082

Depends on #1065

Currently AsVarULE has an as_unaligned() that imposes the requirement that AsVarULE types contian their VarULE counterparts somehow. This is fine for the current set of VarULE types, but in general does not work for e.g. custom VarULE types like

struct Relation {
   something: u8,
   something_else: u32,
   data: VarZeroVec<String>
}

#[repr(packed)]
struct RelationULE {
   something: u8::ULE,
   something_else: u32::ULE,
   data: [u8]
}

We currently only require as_unaligned() for the PartialEq implementation, which we can get around by adding some comparison methods to AsVarULE.

At a more fundamental level, it's worth considering if AsVarULE is worth it in the first place: VarZeroVec<str> might be just as convenient. We only need AsULE for ZeroVec for convenient conversions, but VarZeroVec promises to provide usable values for a lot of its use cases. Perhaps it is better to get rid of the distinction entirely.

cc @zbraniecki @sffc

This is needed by @zbraniecki for #615

@Manishearth Manishearth added C-data-infra Component: provider, datagen, fallback, adapters S-medium Size: Less than a week (larger bug fix or enhancement) labels Sep 22, 2021
@Manishearth Manishearth self-assigned this Sep 22, 2021
@Manishearth Manishearth added the blocked A dependency must be resolved before this is actionable label Sep 22, 2021
@sffc
Copy link
Member

sffc commented Sep 23, 2021

Related: if we change VarZeroVec<String> to VarZeroVec<str> and VarZeroVec<Vec<u8>> to VarZeroVec<[u8]>, then we also need to change ZeroMap<String, Vec<u8>> to ZeroMap<str, [u8]>. (follow-up to #1057 (comment))

@Manishearth
Copy link
Member Author

Yep!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked A dependency must be resolved before this is actionable C-data-infra Component: provider, datagen, fallback, adapters S-medium Size: Less than a week (larger bug fix or enhancement)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants