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

Define a macro to create self-referencing type and use it for font face #184

Closed
wants to merge 1 commit into from

Conversation

EHfive
Copy link
Contributor

@EHfive EHfive commented Sep 17, 2023

This makes unsafe part be more concentrated and allows reuse. The macro would be used in later patch.

This makes unsafe part be more concentrated and allows reuse.
The macro would be used in later patch.
@hecrj
Copy link
Contributor

hecrj commented Sep 18, 2023

Have we considered using ouroboros instead of rolling our own unsafe?

@EHfive
Copy link
Contributor Author

EHfive commented Sep 18, 2023

Have we considered using ouroboros instead of rolling our own unsafe?

There was, but dropped for compilation time reason. 95e3624 Though it's not a big issue for me..

The produced type is basically equivalent to expansion of ouroboros macro so I think it's fine.

And this macro should still be faster than ouroboros as we didn't use proc-macro here.

@Imberflur
Copy link
Contributor

Imberflur commented Sep 18, 2023

Imo if this will be using custom unsafe, then it is easier to verify the soundness of a particular implementation rather than a generic macro. And I don't see any other motivation for a macro in this case other than encapsulating the unsafe code (edit: Sorry, I missed the mention of the macro being used in a later patch. I still think the correctness of the macro is more difficult to verify, maybe it isn't too bad...)

Also, the current implementation has a soundness issue, but it is not too surprising since this was just fixed in ouroboros someguynamedjosh/ouroboros#88. I can make a PR for this later today.

Comment on lines +11 to +14
mod owned_face {
impl_self_ref!(OwnedFace, rustybuzz::Face<'static>, rustybuzz::Face<'this>);
}
use owned_face::*;
Copy link
Contributor

Choose a reason for hiding this comment

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

wrapping this up in a module is necessary to prevent accessing the fields incorrectly so it should probably be part of the macro

@EHfive
Copy link
Contributor Author

EHfive commented Sep 18, 2023

@Imberflur After roughly look at the issue you mentioned, and given the motivation of not using ouroboros is to speed up compilation. I think we should just use the self_cell which is proc-macro-free thus being faster to compile. And to avoid implementing indigenous unsafe code here which has a lesser chance to be audited.

/// this struct
pub struct $SelfRef<T> {
/// `data_ref` could self-referencing `data`
data_ref: $RefStatic,
Copy link
Contributor

Choose a reason for hiding this comment

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

This field needs to be wrapped in maybe_dangling::MaybeDangling or core::mem::MaybeUninit to prevent the compiler from potentially assuming any references it contains will be valid till the end of a function when $SelfRef<T> is passed as a function parameter (see the issue I linked above). Any such references will only be valid until this value is dropped.

@EHfive EHfive closed this Sep 18, 2023
@EHfive EHfive deleted the add-self-ref-macro branch September 18, 2023 17:16
@Imberflur
Copy link
Contributor

Imberflur commented Sep 18, 2023

@Imberflur After roughly look at the issue you mentioned, and given the motivation of not using ouroboros is to speed up compilation. I think we should just use the self_cell which is proc-macro-free thus being faster to compile. And to avoid implementing indigenous unsafe code here which has a lesser chance to be audited.

That definitely seems like the best approach. We could have removed the extra layer of indirection from the AliasableBox here (and the equivalent in self_cell), since an Arc already wraps the inner data but I think it is much more valuable to rely on unsafe code that will have a higher chance of being well audited.

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.

3 participants