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

[derive] Improve repr parsing #675

Open
joshlf opened this issue Dec 5, 2023 · 0 comments
Open

[derive] Improve repr parsing #675

joshlf opened this issue Dec 5, 2023 · 0 comments

Comments

@joshlf
Copy link
Member

joshlf commented Dec 5, 2023

Context

As described in #672 (review) (text reproduced below), we should overhaul zerocopy-derive's parsing of repr attributes. We opted to merge #672, but this is still important follow-up work.

Original text

This is the text of #672 (review)

This PR partially repairs a longstanding oddity in our repr parsing: the special treatment of Align. align does not appear in our list of allowed combinations:

const STRUCT_UNION_ALLOWED_REPR_COMBINATIONS: &[&[StructRepr]] = &[
&[StructRepr::C],
&[StructRepr::Transparent],
&[StructRepr::Packed],
&[StructRepr::C, StructRepr::Packed],
];

...nor the invocation of define_kind_specific_repr:
define_kind_specific_repr!("a struct", StructRepr, C, Transparent, Packed);

Rather, it is defined implicitly during macro expansion:

pub enum $repr_name {
$($repr_variant,)*
Align(u64),
}

This is because, unlike the other modifiers, align always takes an argument. This PR makes define_kind_specific_repr a little less magical: It extends the macro with the ability to generate provided argument-taking modifiers.

However, the way it fixes the issue is still a little magical. Recall that validate_reprs filters out the Align repr (because it would be a pain to enumerate these in allowed_combinations):

metas_reprs.into_iter().filter(|(_, repr)| !repr.is_align()).for_each(|(meta, repr)| {
metas.push(meta);
reprs.push(repr)
});

This PR leverages that behavior by having PackedN(n).is_align() return true:

fn is_align(&self) -> bool {
match self {
$($repr_name::$repr_variant_aligned(_) => true,)*
_ => false,
}
}

I'm not entirely comfortable with this. Not only is the name misleading now, but what happens when a new argument-consuming is added (particularly one that doesn't modify alignment)?

@joshlf, I think we need to reconsider our repr parsing framework, and reduce the the special handling of Align and PackedN. My understanding is we need this special handling because we can't write out argument-bearing modifiers in allowed_combinations. Perhaps we can abstract the argument component so we can specify either concrete or pattern arguments; e.g.:

// ditto for `StructRepr`, etc.
enum ReprKind<A: ReprArgument> {
    C,
    Transparent,
    Packed,
    PackedN(A::PackedN),
    Align(A::Align),
}

trait ReprArgument {
    type PackedN;
    type Align,
}

struct Pattern;
struct Concrete;

impl ReprArgument for Pattern {
    type PackedN = u64;
    type Align = u64;
}

impl ReprArgument for Concrete {
    type PackedN = RangeFull;
    type Align = RangeFull;
}

const STRUCT_UNION_ALLOWED_REPR_COMBINATIONS: &[&[StructRepr<Pattern>]] = &[
    &[StructRepr::C],
    &[StructRepr::Transparent],
    &[StructRepr::Packed],
    &[StructRepr::PackedN(..)],
    &[StructRepr::Align],
    &[StructRepr::C, StructRepr::Packed],
    &[StructRepr::C, StructRepr::Packed(..)],
    &[StructRepr::C, StructRepr::Align],
];
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

No branches or pull requests

1 participant