Skip to content
This repository has been archived by the owner on Jan 14, 2023. It is now read-only.

Added StaticDataSize trait and derive macros for DataSize and StaticDataSize #1

Merged
merged 22 commits into from
Nov 28, 2022

Conversation

Syudagye
Copy link
Contributor

It is now possible to implement StaticDataSize and DataSize traits with derive macro !

// Both enums and structs with named or unnamed fields are supported

// Deriving for StaticDataSize will also implement DataSize
// You don't need to add a derive for DataSize (It will not compile anyways)
#[derive(StaticDataSize)] // The size is known at compile time
enum TestSizedEnum {
	Unit,
	Unnamed(u16),
	Named { field1: u32, field2: i8 },
}
#[derive(DataSize)] // The size will change
struct TestDynamicTuple(Vec<Option<u64>>, i64);

// Even Generics works !!!
#[derive(DataSize, StaticDataSize)] // Here T could be statically sized or not, so we can derive both traits
struct TestStructGenerics<'a, T> {
	value: &'a T,
	wrapper: Option<T>,
	enum_value: TestEnumGenerics<Option<T>>,
}

The only thing left to do is the documentation :)

Copy link
Collaborator

@Antikyth Antikyth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good! There are some things that I think need to be changed (see comments), and some that could be changed but mostly just for readability :)

datasize_macro/Cargo.toml Outdated Show resolved Hide resolved
datasize_macro/src/impl_data_sizes.rs Outdated Show resolved Hide resolved
datasize_macro/src/impl_data_sizes.rs Outdated Show resolved Hide resolved
datasize_macro/src/impl_data_sizes.rs Outdated Show resolved Hide resolved
datasize_macro/src/impl_data_sizes.rs Outdated Show resolved Hide resolved
datasize_macro/src/lib.rs Outdated Show resolved Hide resolved
datasize_macro/src/lib.rs Outdated Show resolved Hide resolved
src/datasize.rs Outdated Show resolved Hide resolved
src/datasize.rs Outdated Show resolved Hide resolved
src/datasize.rs Outdated Show resolved Hide resolved
@Antikyth
Copy link
Collaborator

I didn't know about the specialization feature - that seems really useful, especially for this kinda thing! Is it a concern that you have to even allow a lint for incomplete_features on top of enabling the feature flag though? Sounds like this feature is likely to change or possibly even be removed?

@Syudagye
Copy link
Contributor Author

I didn't know about the specialization feature - that seems really useful, especially for this kinda thing! Is it a concern that you have to even allow a lint for incomplete_features on top of enabling the feature flag though? Sounds like this feature is likely to change or possibly even be removed?

Yes that is really useful for this right now.
But yes it is not complete, there is an issue with "soundness" if i remember well:
https://rust-lang.github.io/rfcs/1210-impl-specialization.html
rust-lang/rust#31844
rust-lang/rfcs#1210

@Antikyth
Copy link
Collaborator

By the way, just a terminology thing: lifetimes are generics. In the same way that generic types represent a non-specific type with certain bounds (such as the places that generic type is put), lifetimes are generic in that they don't represent a specific length of time, rather just a non-specific length of time, where you may annotate that certain things live for at least the same lifetime as others by placing lifetimes in those positions. So 'generics' refers to both generic types and lifetimes :)

Copy link
Collaborator

@Antikyth Antikyth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not done reviewing, will finish probably tomorrow. Looks good! Maybe one or two minor things I guess, too tired to read properly now :p

.collect();

quote! {
Self::#ident {#(#names),*} => { 0usize #( + #names.data_size())*},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize this should be 1usize right? To include the discriminant?

datasize_macro/src/impl_data_sizes.rs Outdated Show resolved Hide resolved
datasize_macro/src/lib.rs Outdated Show resolved Hide resolved
@Antikyth Antikyth merged commit 40bdff5 into XdotRS:master Nov 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants