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

Refactor fold #283

Merged
merged 10 commits into from Nov 12, 2019

Conversation

@nikomatsakis
Copy link
Collaborator

nikomatsakis commented Nov 12, 2019

Refactor the fold crate to take a Target Family, so we now have Fold<TF, TTF>, where TF is the source target family and TTF is the target type family.

As part of this change, we no longer use TF::Type to reference types. Instead, we use Ty<TF> (and Lifetime<TF>). This is both more uniform (you always do Foo<TF> now no matter what), it also allows Ty<TF> to implement for<TTF> Fold<TF, TTF>, something which couldn't be expressed otherwise owing to the fact that Rust doesn't use Chalk yet.

I'm putting this as r? @detrumi because they are probably the one most familiar with fold after writing the derive, but @detrumi if you prefer I can try to find another reviewer. =)

We now use `Ty<TF>` to refer to a "type", and `TyData<TF>` to refer to
the type data. The `Ty<TF>` type implements the `Fold`, `Zip`, and
other traits directly (instead of `TF::Type` implementing them).

`TF::Type` is renamed to `InternedType`.
This matches the changes introduced for `Ty`
They are going to diverge more in future commits
also, document fold a bit more
Copy link
Contributor

detrumi left a comment

Nice improvements, though I'd personally give the TTF parameter on Fold a default to make it a bit more readable.

And did you make most Fold impl's symmetric only because it's not needed for a TTF != TF yet, or is there a reason to avoid making them too general?

chalk-ir/src/lib.rs Show resolved Hide resolved
chalk-ir/src/lib.rs Show resolved Hide resolved
chalk-ir/src/family.rs Outdated Show resolved Hide resolved
chalk-derive/src/lib.rs Show resolved Hide resolved
chalk-ir/src/fold.rs Outdated Show resolved Hide resolved
chalk-ir/src/fold.rs Show resolved Hide resolved
@nikomatsakis

This comment has been minimized.

Copy link
Collaborator Author

nikomatsakis commented Nov 12, 2019

OK, I applied the feedback.

@nikomatsakis

This comment has been minimized.

Copy link
Collaborator Author

nikomatsakis commented Nov 12, 2019

Er, I pushed to the wrong branch. Let me fix that.

@nikomatsakis nikomatsakis merged commit f9658ca into rust-lang:master Nov 12, 2019
1 check passed
1 check passed
Travis CI - Pull Request Build Passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.