-
Notifications
You must be signed in to change notification settings - Fork 13.5k
Port #[target_feature]
to new attribute parsing infrastructure
#142876
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -228,7 +228,10 @@ pub(crate) enum AttributeOrder { | |||||||||
KeepLast, | ||||||||||
} | ||||||||||
|
||||||||||
type ConvertFn<E> = fn(ThinVec<E>) -> AttributeKind; | ||||||||||
pub(crate) enum ConvertFn<E> { | ||||||||||
Simple(fn(ThinVec<E>) -> AttributeKind), | ||||||||||
WithFirstAttributeSpan(fn(ThinVec<E>, Span) -> AttributeKind), | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Document this variant |
||||||||||
} | ||||||||||
|
||||||||||
/// Alternative to [`AttributeParser`] that automatically handles state management. | ||||||||||
/// If multiple attributes appear on an element, combines the values of each into a | ||||||||||
|
@@ -262,22 +265,36 @@ pub(crate) trait CombineAttributeParser<S: Stage>: 'static { | |||||||||
pub(crate) struct Combine<T: CombineAttributeParser<S>, S: Stage>( | ||||||||||
PhantomData<(S, T)>, | ||||||||||
ThinVec<<T as CombineAttributeParser<S>>::Item>, | ||||||||||
Option<Span>, | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. May be worth naming the fields now (and documenting them) |
||||||||||
); | ||||||||||
|
||||||||||
impl<T: CombineAttributeParser<S>, S: Stage> Default for Combine<T, S> { | ||||||||||
fn default() -> Self { | ||||||||||
Self(Default::default(), Default::default()) | ||||||||||
Self(Default::default(), Default::default(), Default::default()) | ||||||||||
} | ||||||||||
} | ||||||||||
|
||||||||||
impl<T: CombineAttributeParser<S>, S: Stage> AttributeParser<S> for Combine<T, S> { | ||||||||||
const ATTRIBUTES: AcceptMapping<Self, S> = &[( | ||||||||||
T::PATH, | ||||||||||
<T as CombineAttributeParser<S>>::TEMPLATE, | ||||||||||
|group: &mut Combine<T, S>, cx, args| group.1.extend(T::extend(cx, args)), | ||||||||||
|group: &mut Combine<T, S>, cx, args| { | ||||||||||
// Keep track of the span of the first attribute, for diagnostics | ||||||||||
if group.2.is_none() { | ||||||||||
group.2 = Some(cx.attr_span); | ||||||||||
} | ||||||||||
Comment on lines
+283
to
+285
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
group.1.extend(T::extend(cx, args)) | ||||||||||
}, | ||||||||||
)]; | ||||||||||
|
||||||||||
fn finalize(self, _cx: &FinalizeContext<'_, '_, S>) -> Option<AttributeKind> { | ||||||||||
if self.1.is_empty() { None } else { Some(T::CONVERT(self.1)) } | ||||||||||
if let Some(first_span) = self.2 { | ||||||||||
Some(match T::CONVERT { | ||||||||||
ConvertFn::Simple(f) => f(self.1), | ||||||||||
ConvertFn::WithFirstAttributeSpan(f) => f(self.1, first_span), | ||||||||||
}) | ||||||||||
} else { | ||||||||||
None | ||||||||||
} | ||||||||||
Comment on lines
-281
to
+298
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This now always invokes the function, even if the vec is empty, is that expected? |
||||||||||
} | ||||||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed
ConvertFn
to an enum so I can also keep track of the span of the first attribute. I use this span to lint on the attribute itselfs (rather than one of the elements) for a few of the diagnostics. Does this seem reasonsable?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm... we could either just always pass an
Option<Span>
or just always pass aSpan
and let the user decide whether aDUMMY_SP
is something they want to ignore in general