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

Parse unnamed fields and anonymous structs or unions #115732

Closed
wants to merge 1 commit into from
Closed

Parse unnamed fields and anonymous structs or unions #115732

wants to merge 1 commit into from

Conversation

frank-king
Copy link
Contributor

This PR implements #49804, and parses the anonymous structs or unions as a special type of field, instead of a general kind of Ty.

r? @petrochenkov

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 10, 2023
@rustbot
Copy link
Collaborator

rustbot commented Sep 10, 2023

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

Changes to the size of AST and/or HIR nodes.

cc @nnethercote

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@frank-king frank-king mentioned this pull request Sep 11, 2023
Merged
5 tasks
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rustbot
Copy link
Collaborator

rustbot commented Sep 11, 2023

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@frank-king frank-king mentioned this pull request Sep 20, 2023
Closed
@petrochenkov
Copy link
Contributor

This PR implements two independent changes

  • The TyKind::Anon{Struct,Union} -> FieldTy refactoring, which probably make sense as a cleanup
  • The support for unnamed fields in built-in derives, which is a functional change, and probably doesn't make much sense until the unnamed fields are actually implemented (in HIR and further).

Could you keep only the first part in this PR?
The second one can be submitted separately and later.
@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 7, 2023
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors

This comment was marked as resolved.

Sign in to view
@petrochenkov
Copy link
Contributor

@frank-king
Could you set the S-waiting-on-review label when this PR is rebased and ready?
It can be done with @rustbot ready and other similar commands.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors

This comment was marked as resolved.

Sign in to view
@frank-king frank-king mentioned this pull request Nov 17, 2023
Open
@rustbot
Copy link
Collaborator

rustbot commented Nov 19, 2023

Some changes occurred in src/tools/cargo

cc @ehuss

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@frank-king
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 19, 2023
@bors

This comment was marked as resolved.

Sign in to view
@frank-king
Parse unnamed fields and anonymous structs or unions
2f6a16d 
Anonymous structs or unions are parsed as a special type of field,
instead of a gerenal kind of `Ty`.
@petrochenkov
Copy link
Contributor

I wonder if this refactoring (special kind of field, instead of a type) is an improvement at all, if the resulting diff is +416 −158.
Let me check in more detail.

@frank-king
Copy link
Contributor Author

The problem is that, since AnonStruct and AnonUnion is not regarded as a TyKind at all, everywhere it should be reconsidered as a special kind of struct field where it appears.

The current approach (before this PR) requires more efforts on parsing anonymous structs or unions as a special TyKind, but after PR, we need more efforts on the special FieldTy, which is the side effect.

@petrochenkov
Copy link
Contributor

Yeah, I'm not convinced such refactoring is an improvement.
Everything becomes more complex, and even ast_validation.rs doesn't become simpler.

Work on RFC 2102 needs to rather focus on implementing the feature further in middle end and code generation.
Maybe after the feature is complete we can consider refactoring the representation to fit it better.

I'm going to close this in meantime.

@frank-king
Copy link
Contributor Author

Well, I'll then continue to work on #115367

@frank-king frank-king mentioned this pull request Mar 4, 2024
Open
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nnethercote nnethercote nnethercote left review comments

Assignees

@petrochenkov petrochenkov

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@frank-king @rustbot @rust-log-analyzer @petrochenkov @bors @nnethercote