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

Add initial zeroslice! and zerovec! macros #3453

Merged
merged 2 commits into from
May 26, 2023

Conversation

skius
Copy link
Member

@skius skius commented May 25, 2023

Part of #1935.

This adds the zeroslice![] and zerovec![] macros:

const S: &ZeroSlice<char> = zeroslice![char; <char as AsULE>::ULE::from_array; 'e'];

A follow-up PR will use this macro in ICU4X where applicable.

@skius skius requested review from Manishearth and sffc as code owners May 25, 2023 12:03
@skius skius requested a review from robertbastian May 25, 2023 12:03
Copy link
Member

@robertbastian robertbastian left a comment

Choose a reason for hiding this comment

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

Nice!

() => (
$crate::ZeroSlice::new_empty()
);
($aligned:ty; $array_fn:expr; $($x:expr),+ $(,)?) => (
Copy link
Member

Choose a reason for hiding this comment

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

thought: add a case without $array_fn that uses <$aligned as $crate::ule::AsULE>::ULE::from_array.

Copy link
Member Author

Choose a reason for hiding this comment

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

Please have a look at #3454

Copy link
Member

Choose a reason for hiding this comment

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

is the type strictly necessary? We could probably make do with using _ if the type is omitted and rely on context

Copy link
Member

Choose a reason for hiding this comment

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

We need the unaligned type to declare the const slice, and the cleanest way to get that is to ask for the aligned type.

Copy link
Member Author

@skius skius May 25, 2023

Choose a reason for hiding this comment

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

To expand on that, the internal const X is necessary for non-const contexts:

let x: &ZeroSlice<char> = zeroslice![<char as AsULE>::ULE::from_array; 'a'];
println!("{:?}", x); // error: creates a temporary value which is freed while still in use ^

Copy link
Member

@sffc sffc May 26, 2023

Choose a reason for hiding this comment

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

Fortunately or unfortunately, I can get the compiler to extend the lifetime of the inner array (avoiding the need for the temporary) by using transmute directly instead of calling the ZeroSlice constructor...

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=2ea56560dc8e192e744faf2f1abe60bb

Copy link
Member

Choose a reason for hiding this comment

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

Would like @Manishearth's thoughts on if there is some other way we can avoid the type. It's totally something the compiler should be able to infer; I just don't know how in this context

Copy link
Member

Choose a reason for hiding this comment

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

I don't think that code is sound. You're extending the lifetime of the type, but the temporary array still gets dropped at the end of the expression. In fact if you run Miri you get

error: Undefined Behavior: pointer to alloc8209 was dereferenced after this allocation got freed

Copy link
Member

Choose a reason for hiding this comment

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

Inline const can solve this in the future. However in the follow up we're using the type to look up the conversion function, and I'd rather specify the type than the function.

Copy link
Member

@robertbastian robertbastian left a comment

Choose a reason for hiding this comment

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

I think this is a good start, and @skius is already working on removing the $array_fn parameter in #3454.

Copy link
Member

@sffc sffc left a comment

Choose a reason for hiding this comment

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

So happy to see this coming together

@robertbastian robertbastian merged commit 1baf9e9 into unicode-org:main May 26, 2023
@Manishearth Manishearth mentioned this pull request Sep 21, 2023
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants