-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Refactor deriving code between auto_encode.rs and deriving.rs #5090
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
Labels
A-syntaxext
Area: Syntax extensions
C-cleanup
Category: PRs that clean code up or issues documenting cleanup.
Comments
I left a few FIXMEs scattered about. |
Is there a particular reason why the traits in #[deriving(Eq, Clone, Encodable, Decodable)]
struct Foo { bar: int } |
@dbaupp no, they should be. I think @erickt had started some work in that direction, actually. |
bors
added a commit
that referenced
this issue
Apr 12, 2013
…inger This refactors much of the ast generation required for `deriving` instances into a common interface, so that new instances only need to specify what they do with the actual data, rather than worry about naming function arguments and extracting fields from structs and enum. (This all happens in `generic.rs`. I've tried to make sure it was well commented and explained, since it's a little abstract at points, but I'm sure it's still a little confusing.) It makes instances like the comparison traits and `Clone` short and easy to write. Caveats: - Not surprisingly, this slows the expansion pass (in some cases, dramatically, specifically deriving Ord or TotalOrd on enums with many variants). However, this shouldn't be too concerning, since in a more realistic case (compiling `core.rc`) the time increased by 0.01s, which isn't worth mentioning. And, it possibly slows type checking very slightly (about 2% worst case), but I'm having trouble measuring it (and I don't understand why this would happen). I think this could be resolved by using traits and encoding it all in the type system so that monomorphisation handles everything, but that would probably be a little tricky to arrange nicely, reduce flexibility and make compiling rustc take longer. (Maybe some judicious use of `#[inline(always)]` would help too; I'll have a bit of a play with it.) - The abstraction is not currently powerful enough for: - `IterBytes`: doesn't support arguments of type other than `&Self`. - `Encodable`/`Decodable` (#5090): doesn't support traits with parameters. - `Rand` & `FromStr`; doesn't support static functions and arguments of type other than `&Self`. - `ToStr`: I don't think it supports returning `~str` yet, but I haven't actually tried. (The last 3 are traits that might be nice to have: the derived `ToStr`/`FromStr` could just read/write the same format as `fmt!("%?", x)`, like `Show` and `Read` in Haskell.) I have ideas to resolve all of these, but I feel like it would essentially be a simpler version of the `mt` & `ty_` parts of `ast.rs`, and I'm not sure if the simplification is worth having 2 copies of similar code. Also, makes Ord, TotalOrd and TotalEq derivable (closes #4269, #5588 and #5589), although a snapshot is required before they can be used in the rust repo. If there is anything that is unclear (or incorrect) either here or in the code, I'd like to get it pointed out now, so I can explain/fix it while I'm still intimately familiar with the code.
Closed as part of #6851 |
bors
added a commit
to rust-lang-ci/rust
that referenced
this issue
May 2, 2020
…phansch Split up `match_same_arms` ui test Part of rust-lang#2038 changelog: none
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
A-syntaxext
Area: Syntax extensions
C-cleanup
Category: PRs that clean code up or issues documenting cleanup.
Both auto_encode and deriving.rs do similar things and share various similar routines. We should probably write a "deriving utility library" that they can all use. Deriving already seems to be factored in this way, so maybe it's just a matter of making auto_encode use the routines found there.
The text was updated successfully, but these errors were encountered: