Skip to content

Put impls of primitives near the traits they implement in std #12925

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

Closed
brson opened this issue Mar 16, 2014 · 17 comments · Fixed by #14464
Closed

Put impls of primitives near the traits they implement in std #12925

brson opened this issue Mar 16, 2014 · 17 comments · Fixed by #14464
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.

Comments

@brson
Copy link
Contributor

brson commented Mar 16, 2014

In std there are modules for the primitive types and these modules tend to contain the impls for several of the most 'core' Rust traits. I would rather these live in the module that declares the trait. std is going to continue to be refactored, and the crates these traits live in may change. Having the primitive impls in one place makes that refactoring easier.

@brson
Copy link
Contributor Author

brson commented Mar 16, 2014

This also lets us delete some modules that are nothing but impls.

@Kimundi
Copy link
Member

Kimundi commented Mar 16, 2014

I started working on this

@Kimundi
Copy link
Member

Kimundi commented Mar 16, 2014

Turns out I read the description wrong and spend the whole day refactoring primitive impls away from their traits.

@brson , I don't suppose this could still be useful: https://github.com/Kimundi/rust/compare/move_impls?expand=1 ?

@brson
Copy link
Contributor Author

brson commented Mar 16, 2014

@Kimundi That is indeed the opposite of what I wanted :(

@malu
Copy link

malu commented Apr 1, 2014

If this is still relevant (it seems to be) and @Kimundi stopped working on this I could work on this.

@Kimundi
Copy link
Member

Kimundi commented Apr 1, 2014

@malu Sorry, should've made it clear that I'm not working on it any more.

@kud1ing
Copy link

kud1ing commented Apr 3, 2014

I've always found it surprising that implementations for independent types are bundled in the module where the interface is defined.

Is refactorability the only reason for this?

@thestinger
Copy link
Contributor

It's very convenient because you can usually reuse a lot of code with a macro. It's not relevant to the user-facing API at all.

@sfackler
Copy link
Member

sfackler commented Apr 3, 2014

You also avoid ending up with blank modules like this: http://static.rust-lang.org/doc/master/std/unit/index.html

@malu
Copy link

malu commented Apr 3, 2014

@kud1ing I guess it's also nice to have impls for standard types near trait def because it like saying "Look, this trait is actually useful and can be fulfilled easily by these standard types."

@tbu-
Copy link
Contributor

tbu- commented Apr 16, 2014

@malu Still working on it?

@malu
Copy link

malu commented Apr 17, 2014

@tbu- Yes. Sorry, the past day were pretty busy, I hope I can do a pull request this weekend after rebasing.

@malu
Copy link

malu commented May 4, 2014

Okay, something went horribly wrong during rebasing. I don't think I'm going to be able to fix this anytime soon. :(
So anybody should feel free to work on this.

@flaper87
Copy link
Contributor

flaper87 commented May 4, 2014

@malu sorry to hear that :( Any chance you can upload your work pre-rebase so that people working on this change can take over based on your changes (or at least use them as reference) ?

@huonw
Copy link
Member

huonw commented May 4, 2014

You can use git reflog to "reset" to a previous branch state, restoring you back to how it was before.

@ahmedcharles
Copy link
Contributor

Is this still worth working on?

@Sawyer47
Copy link
Contributor

I could work on this. I think it's worth it: core::bool would go away (to_bit function is pretty useless), core::num::{i*,u*,f*} would only contain constants and could be easily removed in the future. Plus implementing one trait for many types with macro will probably result in shorter code.

(well, I started working on this, let me know if someone else is working on this too)

Sawyer47 added a commit to Sawyer47/rust that referenced this issue May 28, 2014
bors added a commit that referenced this issue May 28, 2014
This is an attempt of fixing #12925.

This PR moves almost all trait implementations for primitive types ((), bool, char, i*, u*, f*) near trait definitions. Only Float trait implementations weren't moved because they heavily rely on constants defined in f32.rs and f64.rs.

Some trait implementations had cfg(not(test)) attribute. I suspect it's because of issue 2912.
Still, someone who knows the problem better should probably check this code.

Closes #12925
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.