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

Disallow ref structs in expression trees #30871

Merged
merged 16 commits into from
Nov 5, 2018
Merged

Disallow ref structs in expression trees #30871

merged 16 commits into from
Nov 5, 2018

Conversation

RikkiGibson
Copy link
Contributor

@RikkiGibson RikkiGibson commented Oct 31, 2018 •
edited
Loading

Resolves #30776

This adds the error code ErrorCode.ERR_ExpressionTreeCantContainRefStruct.

@RikkiGibson RikkiGibson requested review from agocke and gafter October 31, 2018 20:51
@RikkiGibson RikkiGibson requested a review from a team as a code owner October 31, 2018 20:51
@RikkiGibson
Copy link
Contributor Author

RikkiGibson commented Oct 31, 2018 •
edited
Loading

On the topic of parameters: ref struct is already prohibited for generic arguments, which means it can't be used in Action<T> but could be used in delegate void MyDelegate(MyRefStruct s). Should it even be allowed in delegate parameter/return types?

Resolution: Allowed parameter and return types for delegates will not change.

@RikkiGibson
Copy link
Contributor Author

DiagnosticsPass_ExpressionTrees.cs seems like it might be a more appropriate location than this walker in the middle of lambda binding.

@RikkiGibson RikkiGibson force-pushed the ref-struct-expression-tree-emit branch from 0ed7306 to e04bc57 Compare October 31, 2018 23:16
gafter
gafter reviewed Nov 1, 2018
View reviewed changes
Copy link
Member

@gafter gafter left a comment

Choose a reason for hiding this comment

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

Done with review pass (iteration 8)

@RikkiGibson
Copy link
Contributor Author

Resolved feedback. Please have another look.

agocke
agocke approved these changes Nov 5, 2018
View reviewed changes
Copy link
Member

@agocke agocke left a comment

Choose a reason for hiding this comment

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

LGTM

I forgot BoundTreeWalkerWithStackGuard prevents you from hooking VisitExpression so it can insert the stack guard.

@RikkiGibson RikkiGibson requested a review from a team November 5, 2018 22:37
@RikkiGibson
Copy link
Contributor Author

@dotnet/roslyn-compiler please have a look

@RikkiGibson RikkiGibson merged commit ed8a00d into dotnet:master Nov 5, 2018
@RikkiGibson RikkiGibson deleted the ref-struct-expression-tree-emit branch November 5, 2018 23:14
roji pushed a commit to npgsql/efcore.pg that referenced this pull request Feb 28, 2019
@Brar @roji
Remove AsSpan() call from CharacterValueComparer expression
00d0e70 
This optimization is no longer valid.
See dotnet/roslyn#30871
@jjonescz jjonescz mentioned this pull request Jun 25, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaredpar jaredpar jaredpar left review comments

@gafter gafter gafter left review comments

@agocke agocke agocke approved these changes

@cston cston cston approved these changes

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

Successfully merging this pull request may close these issues.
5 participants
@RikkiGibson @jaredpar @agocke @gafter @cston