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

Block delegate*<void>...enum constants in attributes #66073

Merged
merged 24 commits into from
Jan 13, 2023
Merged

Block delegate*<void>...enum constants in attributes #66073

merged 24 commits into from
Jan 13, 2023

Conversation

jjonescz
Copy link
Member

@jjonescz jjonescz commented Dec 20, 2022 •
edited
Loading

Closes #65594.
Follow up on #50903.

TypeNameSerializer cannot handle function pointers (adding support is tracked in #48765). This PR blocks other scenarios where this would result in a stack overflow - when using a constant of an enum where the enum is nested in a type referencing a function pointer.

@jjonescz jjonescz marked this pull request as ready for review December 20, 2022 13:01
@jjonescz jjonescz requested a review from a team as a code owner December 20, 2022 13:01
@AlekseyTs
Copy link
Contributor

Done with review pass (commit 3)

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 9)

@jjonescz jjonescz changed the title Block default(delegate*<void>...enum) in attributes Block delegate*<void>...enum constants in attributes Jan 2, 2023
@jjonescz jjonescz mentioned this pull request Jan 3, 2023
Closed
@AlekseyTs AlekseyTs dismissed cston’s stale review January 10, 2023 15:51

It looks like the implementation got changed significantly since the sign-off

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 18), tests are not looked at

@jjonescz
Copy link
Member Author

Thanks @AlekseyTs. I have addressed your comments.

tests are not looked at

Tests have been changed only slightly, see this diff: 4d00540#diff-ebf2265ce3e451cdaabcf12d7fdd188aff57df697b7e175e12ee6990c892a423

It looks like some CI jobs did not succeed.

Just some timeouts, doesn't seem related to this PR.

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 21)

This was referenced Jan 12, 2023
Closed
Closed
AlekseyTs
AlekseyTs approved these changes Jan 12, 2023
View reviewed changes
Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (commit 23)

@runfoapp runfoapp bot mentioned this pull request Jan 12, 2023
Open
@jjonescz jjonescz enabled auto-merge (squash) January 13, 2023 08:31
@jjonescz jjonescz merged commit 3b31245 into dotnet:main Jan 13, 2023
@jjonescz jjonescz deleted the 65594-serialize-funcptr branch January 13, 2023 09:55
@ghost ghost added this to the Next milestone Jan 13, 2023
@allisonchou allisonchou mentioned this pull request Jan 18, 2023
Closed
@Cosifne Cosifne modified the milestones: Next, 17.6 P1 Jan 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cston cston cston approved these changes

@AlekseyTs AlekseyTs AlekseyTs approved these changes

Assignees
No one assigned
Labels
Area-Compilers
Projects
None yet
Milestone
17.6 P1
Development

Successfully merging this pull request may close these issues.

[C#] Stack Overflow when a method pointer type appears in an attribute
6 participants
@jjonescz @AlekseyTs @jaredpar @jcouv @cston @Cosifne