Skip to content

Conversation

tmandry
Copy link
Contributor

@tmandry tmandry commented Jan 2, 2024

impl_TCFType! accepts generic types but some of its impls are missing the generics. They work on types with defaulted generics, but only in the default case. This PR fixes that.

It also updates declare_TCFType! to accept generics as a convenience. We use one PhantomData per parameter, as is already done in impl_TCFType!.

In addition, some warnings are silenced, and the macros no longer depend on any traits being in scope in the caller.

@tmandry tmandry changed the title Improve TCFType macros Fix TCFType macros for generic types Jan 29, 2024
@tmandry
Copy link
Contributor Author

tmandry commented Jan 29, 2024

This is needed to create a type-safe generic abstraction over AXValue; see value.rs in eiz/accessibility#5.

@tmandry
Copy link
Contributor Author

tmandry commented Jan 29, 2024

Friendly ping @jdm, would you or another maintainer be willing to take a look at this?

@waywardmonkeys
Copy link
Contributor

@tmandry I'd like to get this unblocked for you. Could you rebase to current main?

Copy link
Contributor

@waywardmonkeys waywardmonkeys left a comment

Choose a reason for hiding this comment

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

Would it be possible to add a test so that it at least remains in a compiling state?

This adds the following new features:

* `declare_TCFType!`: Generics are accepted.
* `impl_TCFType!`: Non-defaulted generics are accepted, and impls are
  fixed to be for all generics.

In addition, some warnings are silenced and the macros no longer depend
on any traits being in scope in the caller.
@tmandry tmandry requested a review from waywardmonkeys August 13, 2024 03:46
@tmandry
Copy link
Contributor Author

tmandry commented Aug 13, 2024

Thanks for taking a look @waywardmonkeys. Should be ready now.

Copy link
Contributor

@waywardmonkeys waywardmonkeys left a comment

Choose a reason for hiding this comment

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

Thanks! I think this looks great! Will leave open for a day or so in case others want to review.

@wusyong wusyong added this pull request to the merge queue Aug 13, 2024
Merged via the queue into servo:main with commit 9d8c74e Aug 13, 2024
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