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.

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

Separate node kind for bindings inside var #4822

Merged
merged 1 commit into from
Jan 21, 2025

Conversation

geoffromer
Copy link
Contributor

This is a step toward making binding pattern handling more robust, by removing its reliance on the node stack for context.

This is a step toward making binding pattern handling more robust, by removing its reliance on the node stack for context.
// whether that pattern is nested inside a `var` pattern.
auto PushStateForPattern(State state, bool in_var_pattern) -> void {
PushState({.state = state,
.in_var_pattern = in_var_pattern,
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm. With this field added, the first two overloads of PushState seem like they're inviting bugs. I wonder if it's worth making them CHECK- or DCHECK-fail when used to push a pattern state.

That said... I suppose the same is true when pushing an expression state, and we seem to not have problems with that. Maybe I'm worrying too much :)

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 agree that this is concerning, but I don't see a way to distinguish pattern states, other than to have a manually-curated list, which seems liable to fall out of date over time. It also seems like that check could be inefficient as the number of pattern states grows, unless we set up something like a perfect hash.

I feel like there's a higher level API to be discovered here, which might help prevent that kind of mistake, but I don't see it clearly yet.

@geoffromer geoffromer enabled auto-merge January 21, 2025 20:02
@geoffromer geoffromer added this pull request to the merge queue Jan 21, 2025
Merged via the queue into carbon-language:trunk with commit 943acf1 Jan 21, 2025
10 checks passed
@geoffromer geoffromer deleted the var-binding branch January 21, 2025 21:25
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