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

condensed duplicate code for trait specialization using macros #424

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hclarke
Copy link
Contributor

@hclarke hclarke commented Aug 1, 2017

I put all the "default" keyword stuff into a impl_fn! macro. that way, there's way less duplicate code for simd trait implementations

I also feel like there should be a "specialization" or "nightly" feature that's separate from "simd" (that simd enables/depends on). This patch doesn't include that, but makes it easier to implement, since there's only one macro to change

also made a BaseSigned trait so that you can use the trait implementation macros for Neg

@hclarke
Copy link
Contributor Author

hclarke commented Aug 2, 2017

ping! @brendanzab

have any thoughts on this one?

I mainly did it because the first PR I submitted failed on the simd travis builds the first time, and I had to fix it by duplicating code. Doing it this way means that changes to the trait implementations only need to happen in one place, so it should reduce friction on future patches

@brendanzab
Copy link
Collaborator

Woopsie, sorry I looked, thought it was awesome, then forgot to reply!

Yeah, I re-read the simd stuff a while back and got a tad confused. In fact I think a whole lot of our macro stuff is a bit ugly, so I'm glad you've been looking into it!

@@ -17,102 +17,148 @@

#![macro_use]

/// generates an impl fn, allowing specialization if simd is enabled
macro_rules! impl_fn {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Think I'd prefer this called impl_operator_method!

/// generates an impl fn, allowing specialization if simd is enabled
macro_rules! impl_fn {


Copy link
Collaborator

Choose a reason for hiding this comment

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

Extra space here!

@hclarke
Copy link
Contributor Author

hclarke commented Aug 9, 2017

these changes are in my queue. will probably get to it in the next few days
thanks for feedback!

@kvark
Copy link
Collaborator

kvark commented Jan 14, 2019

@hclarke looks like the few days got stretched quite a bit :)

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.

None yet

3 participants