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

Add FnDef to TypeName #449

Merged
merged 11 commits into from
May 18, 2020
Merged

Conversation

nathanwhit
Copy link
Member

Addresses the FnDef part of #368.

Lowering and program clause generation are probably the areas that need the most review. I'm also a bit unsure about if/how the builtin traits apply to FnDef, so some guidance on that would be helpful. For parsing I've chosen to only handle function declarations for simplicity's sake (plus I'm not sure why we would need to inspect the function bodies).

In its current state the PR could use some refactoring/polish, but I'll defer that until the more substantive aspects of the PR are in good shape.

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

This looks really good! The main thing I think we might want is to try and cleanup the program clause generation code, but maybe that's better left for another PR.

(I'm also pondering the implied bounds generation logic, but I think it's probably what we want, even though it can't actually be observed by users at the moment.)

chalk-solve/src/clauses/program_clauses.rs Outdated Show resolved Hide resolved
chalk-solve/src/clauses/program_clauses.rs Outdated Show resolved Hide resolved
tests/lowering/mod.rs Show resolved Hide resolved
tests/test/functions.rs Show resolved Hide resolved
@nathanwhit
Copy link
Member Author

Thanks for the helpful feedback!

I've added the implied bounds test and cleaned up the program clause generation code quite a bit, so it should be ready for re-review.

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

Requested a few nits! Looking very close to me.

chalk-solve/src/clauses/program_clauses.rs Outdated Show resolved Hide resolved
chalk-solve/src/clauses/program_clauses.rs Show resolved Hide resolved
chalk-solve/src/clauses/program_clauses.rs Show resolved Hide resolved
chalk-solve/src/clauses/program_clauses.rs Show resolved Hide resolved
chalk-solve/src/clauses/program_clauses.rs Outdated Show resolved Hide resolved
chalk-rust-ir/src/lib.rs Show resolved Hide resolved
chalk-rust-ir/src/lib.rs Show resolved Hide resolved
chalk-rust-ir/src/lib.rs Outdated Show resolved Hide resolved
}

#[derive(Clone, Debug, PartialEq, Eq, Hash, Fold, HasInterner)]
pub struct FnDefDatumBound<I: Interner> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like we are going to need .. well, eventually we'll have to add ABI and other details, but maybe we can add that together with adding the same details to the function pointer types

@nikomatsakis
Copy link
Contributor

r=me but this needs to be rebased.

@nikomatsakis
Copy link
Contributor

And we should probably file a followup issue regarding ABI details (which apply equally to function pointers)

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

r=me but requires rebase and we should break out a follow-up issue regarding handling ABIs, both in these types and function pointers.

@nathanwhit
Copy link
Member Author

Done and done!

@nikomatsakis nikomatsakis merged commit c0daf93 into rust-lang:master May 18, 2020
@nathanwhit nathanwhit deleted the typename-fndef branch May 25, 2020 21:22
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.

2 participants