Skip to content

Define ParserContext as ref struct #259

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

Merged
merged 3 commits into from
Jul 5, 2022
Merged

Define ParserContext as ref struct #259

merged 3 commits into from
Jul 5, 2022

Conversation

Shane32
Copy link
Member

@Shane32 Shane32 commented Jul 4, 2022

No description provided.

@Shane32 Shane32 requested a review from sungam3r July 4, 2022 12:57
@Shane32 Shane32 self-assigned this Jul 4, 2022
@github-actions github-actions bot added the test Pull request that adds new or changes existing tests label Jul 4, 2022
Copy link
Member

@sungam3r sungam3r left a comment

Choose a reason for hiding this comment

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

🤔 It seemed to me that I once tried to do this. Do you have a practical scenario when it is required? Let's add ref to LexerContext too.

@sungam3r sungam3r added performance and removed test Pull request that adds new or changes existing tests labels Jul 5, 2022
@Shane32
Copy link
Member Author

Shane32 commented Jul 5, 2022

🤔 It seemed to me that I once tried to do this. Do you have a practical scenario when it is required? Let's add ref to LexerContext too.

No. It simply is already a ref struct — it is always (and must be) passed by ref; it’s a struct, and it’s internal so the change does not affect the public api or operation but is simply “good housekeeping”

I didn’t look at LexerContext

@Shane32
Copy link
Member Author

Shane32 commented Jul 5, 2022

The change to ref struct prevents accidental storage on the heap, such as seen by the change to the tests

@Shane32 Shane32 merged commit 4121d27 into master Jul 5, 2022
@Shane32 Shane32 deleted the ref_struct branch July 5, 2022 12:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants