Skip to content

Commit

Permalink
[refactor] #2573: clean up comments
Browse files Browse the repository at this point in the history
Signed-off-by: Nikita Strygin <dcnick3@users.noreply.github.com>
  • Loading branch information
DCNick3 authored and mversic committed Oct 17, 2023
1 parent 430224a commit c85392a
Show file tree
Hide file tree
Showing 4 changed files with 11 additions and 17 deletions.
6 changes: 4 additions & 2 deletions ffi/derive/src/attr_parse/derive.rs
Original file line number Diff line number Diff line change
Expand Up @@ -90,8 +90,10 @@ impl FromAttributes for DeriveAttrs {
Derive::Other(path.to_token_stream().to_string())
};

// I __think__ it's an error to use the same derive twice
// I don't think we care in this case though
// Funnily, rust allows the usage of the same derive multiple times
// In most cases this will lead to a "Conflicting implementations of trait" errors,
// but technically it's not an error by itself
// We do handle the duplicate derives just fine
derives.push(derive);
}
}
Expand Down
18 changes: 5 additions & 13 deletions ffi/derive/src/convert.rs
Original file line number Diff line number Diff line change
Expand Up @@ -87,10 +87,9 @@ impl syn2::parse::Parse for SpannedFfiTypeToken {
}
}

/// This represents an `#[ffi_type(...)]` attribute on a type
#[derive(Debug, PartialEq, Eq, Copy, Clone)]
pub enum FfiTypeKindAttribute {
// NOTE: these are not all the possible ffi type kinds, but only those that can be referred to using an attribute
// Enums are treated quite funnily...
Opaque,
UnsafeRobust,
Local,
Expand All @@ -114,6 +113,7 @@ impl syn2::parse::Parse for FfiTypeKindAttribute {
}
}

/// This represents an `#[ffi_type(...)]` attribute on a field
#[derive(Debug, PartialEq, Eq, Copy, Clone)]
pub enum FfiTypeKindFieldAttribute {
UnsafeNonOwning,
Expand Down Expand Up @@ -211,6 +211,7 @@ pub struct FfiTypeInput {
pub ffi_type_attr: FfiTypeAttr,
pub getset_attr: GetSetStructAttrs,
pub span: Span,
/// The original DeriveInput this structure was parsed from
pub ast: syn2::DeriveInput,
}

Expand Down Expand Up @@ -250,14 +251,6 @@ impl darling::FromDeriveInput for FfiTypeInput {
}
}

impl quote::ToTokens for FfiTypeInput {
fn to_tokens(&self, tokens: &mut TokenStream) {
// TODO: this ignores changes that may be done to Generics
// Would this be a problem?
self.ast.to_tokens(tokens);
}
}

#[derive(FromVariant)]
pub struct FfiTypeVariant {
pub ident: syn2::Ident,
Expand Down Expand Up @@ -301,15 +294,14 @@ pub fn derive_ffi_type(emitter: &mut Emitter, input: &syn2::DeriveInput) -> Toke
if variants.is_empty() {
emit!(
emitter,
input.span(),
input.span,
"Uninhabited enums are not allowed in FFI"
);
}
}

// the logic of `is_opaque` is somewhat convoluted and I am not sure if it is even correct
// there is also `is_opaque_struct`...
// NOTE: this mirrors the logic in `is_opaque`. The function should be updated to use darling-parsed attrs together with the call sites
if input.is_opaque() {
return derive_ffi_type_for_opaque_item(name, &input.generics);
}
Expand Down Expand Up @@ -717,7 +709,7 @@ fn derive_ffi_type_for_repr_c(emitter: &mut Emitter, input: &FfiTypeInput) -> To
.kind
.map_or_else(Span::call_site, |kind| kind.span());
// TODO: this error message may be unclear. Consider adding a note about the `#[ffi_type]` attribute
emit!(emitter, span, "The type should be marked with #[repr(C)]");
emit!(emitter, span, "To make an FFI type robust you must mark it with `#[repr(C)]`. Alternatively, try using `#[ffi_type(opaque)]` to make it opaque");
}

let (impl_generics, ty_generics, where_clause) = input.generics.split_for_impl();
Expand Down
1 change: 0 additions & 1 deletion ffi/derive/src/impl_visitor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,6 @@ impl<'ast> Visit<'ast> for ImplVisitor<'ast, '_> {
}
if node.defaultness.is_some() {
emit!(self.emitter, node.defaultness, "Default impl not supported");
// TODO: should we return here, or can the execution continue?
}

for it in &node.attrs {
Expand Down
3 changes: 2 additions & 1 deletion ffi/derive/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,10 +74,11 @@ pub fn ffi(input: TokenStream) -> TokenStream {
.into_iter()
.map(|item| {
if !matches!(item.vis, syn2::Visibility::Public(_)) {
emit!(emitter, item, "Only public types are allowed in FFI");
emit!(emitter, item.span, "Only public types are allowed in FFI");
}

if !item.is_opaque() {
let item = item.ast;
return quote! {
#[derive(iroha_ffi::FfiType)]
#item
Expand Down

0 comments on commit c85392a

Please sign in to comment.