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.

Sign up for GitHub

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

Jump to bottom

Add method to get repr data of an ADT to ChalkDatabase #523

Merged
merged 5 commits into from
Jun 23, 2020
Merged

Add method to get repr data of an ADT to ChalkDatabase #523

merged 5 commits into from
Jun 23, 2020

Conversation

voidc
Copy link
Contributor

@voidc voidc commented Jun 13, 2020

Hi everyone! I'm Dominik and I'm interested in getting involved in rustc and chalk development. #517 seems like a good place to start and get familiar with the code.
With this WIP PR I would like to get some feedback on whether I understood the issue correctly and this is going in the right direction.

closes #517

@voidc voidc mentioned this pull request Jun 13, 2020
Closed
@basil-cow
Copy link
Contributor

Good start, couple of things I want to point out:
a) #[repr(C)] and #[repr(packed)] are not mutually exclusive, so you shouldn't use an enum to represent repr.
b) Storing repr data in AdtDatum is what we wanted to avoid by using callbacks to RustIrDatabase; you need to store it in Program instead.

@voidc
Copy link
Contributor Author

voidc commented Jun 14, 2020

Thanks for the feedback; I revised my PR accordingly. I'm not sure why the tests are failing though.

@basil-cow
Copy link
Contributor

Hmm, not sure, it doesn't like the letter "C" for some reason

@voidc
Copy link
Contributor Author

voidc commented Jun 14, 2020

I think what is going is that due to this line the character C becomes a separate token which breaks occurrences of "C" as an identifier. I'm pretty clueless on how to fix this.

@basil-cow
Copy link
Contributor

I have no idea how lalrpop works internally, @nikomatsakis should know

@nathanwhit
Copy link
Member

I think that (roughly) what happens is that the identifier C becomes a terminal, and so it behaves like a reserved identifier/keyword. It's the same behavior as if, for instance, you declared a struct named impl which the parser would reject because "impl" behaves as a reserved keyword. We don't want this for C, however, since we only care about C (and packed for that matter) in the context of a repr attribute.

I'll leave suggestions to fix it in a separate comment.

nikomatsakis
nikomatsakis reviewed Jun 15, 2020
View reviewed changes
chalk-parse/src/parser.lalrpop Outdated
@@ -58,8 +58,11 @@ WellKnownTrait: WellKnownTrait = {
"#" "[" "lang" "(" "unsize" ")" "]" => WellKnownTrait::Unsize,
};

StructReprC: () = "#" "[" "repr(C)" "]";
StructReprPacked: () = "#" "[" "repr(packed)" "]";
Copy link
Contributor

Choose a reason for hiding this comment

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

I think for this we should just parse any identifier and then generate a list of identifiers in the AST, perhaps, which lowering can match against "C" or "packed", or else have a match statement that examines the identifier in the parser:

StructRepr: XXX = "#" "[" "repr" "(" <name:Ident>")" "]" => {
    match name {
        "C" => ...
        "repr" => ...
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I implemented your suggestion so that StructRepr now parses to an Atom.

@voidc voidc force-pushed the adt-repr branch 2 times, most recently from a1cb5fe to 931d61d Compare June 21, 2020 20:50
@jackh726
Copy link
Member

I think this looks good. @Areredify do you think we need anything other than repr(C) and repr(packed)?

@basil-cow
Copy link
Contributor

basil-cow commented Jun 23, 2020 •
edited
Loading

Not for the foreseeable future I think (ignoring the shared type library part, that is)

@jackh726 jackh726 marked this pull request as ready for review June 23, 2020 17:20
@jackh726 jackh726 merged commit b8a90d8 into rust-lang:master Jun 23, 2020
@nikomatsakis
Copy link
Contributor

Thanks @voidc!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nathanwhit nathanwhit nathanwhit left review comments

@nikomatsakis nikomatsakis nikomatsakis left review comments

@jackh726 jackh726 jackh726 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add callback to get repr data
5 participants
@voidc @basil-cow @nathanwhit @jackh726 @nikomatsakis