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

Add var <type> <identifier> [ = <value> ]; syntax for variables #339

Merged
merged 19 commits into from
Apr 28, 2021
Merged

Add var <type> <identifier> [ = <value> ]; syntax for variables #339

merged 19 commits into from
Apr 28, 2021

Conversation

jonmeow
Copy link
Contributor

@jonmeow jonmeow commented Mar 3, 2021

Adopt var <type> <identifier> [ = <value> ]; syntax for variables

@jonmeow jonmeow added WIP proposal A proposal labels Mar 3, 2021
@jonmeow
Copy link
Contributor Author

jonmeow commented Mar 3, 2021

Proposal links (add links as proposal evolves):

@google-cla google-cla bot added the cla: yes PR meets CLA requirements according to bot. label Mar 3, 2021
@jonmeow jonmeow changed the title var statement var statement Mar 3, 2021
@jonmeow jonmeow added proposal rfc Proposal with request-for-comment sent out and removed WIP labels Mar 3, 2021
code:

1. What is the type of variable `x`?
2. What is the identifier of the `Int` variable?
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, "where is x declared?" I know it was not a part of the quoted study, but I think it is a question that I ask myself often when reading code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the feedback, but I really intended this section to focus on the mirroring of these two questions, and while your suggestion is related I think it's both required to answer either question and would also detract from the contrast.

for type syntax to be more flexible because it can rely on a following syntax
element to disambiguate breaks. However, type syntax may be constrained
sufficiently regardless of `:`. This implies the presence of `:` would mostly be
to address _visual_ ambiguity, not _compiler_ ambiguity.
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you tried out removing the colon from the parser?
You suggest that while it might be ambiguous now, that some other changes might make it unambiguous.
Without suggesting those other changes, this would put us in a messy situation.

Copy link
Contributor

Choose a reason for hiding this comment

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

I gave it a try... removing the colon introduces some shift/reduce conflicts, but that is a mild form of ambiguity. There were no reduce/reduce conflicts, which are the deadly ones. I did not test on all the examples, though I did check one. So my guess is that removing the colon probably is OK from a parsing perspective.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, good to hear.

I would note though, it may just mean that we'd need to go in a different path. In particular, the most issues I'd expect are from syntax like VAR pattern "=" expression ";" -- but I'm not sure whether pattern is the right level of expressivity there (note, this may be my own lack of foresight into the issues).

Copy link
Contributor

@jsiek jsiek left a comment

Choose a reason for hiding this comment

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

I'm OK with the proposed syntax but would prefer that we use the : to reduce the shift/reduce conflicts.

Also, see the comment about type checking and runtime behavior. We all really should be getting into the habit of specifying those aspects as well.

proposals/p0339.md Show resolved Hide resolved
proposals/p0339.md Outdated Show resolved Hide resolved
proposals/p0339.md Outdated Show resolved Hide resolved
proposals/p0339.md Outdated Show resolved Hide resolved
proposals/p0339.md Outdated Show resolved Hide resolved
Co-authored-by: Geoff Romer <[email protected]>
@jonmeow jonmeow requested a review from a team March 29, 2021 21:23
proposals/p0339.md Outdated Show resolved Hide resolved
proposals/p0339.md Show resolved Hide resolved
proposals/p0339.md Show resolved Hide resolved
proposals/p0339.md Show resolved Hide resolved
Copy link
Contributor Author

@jonmeow jonmeow left a comment

Choose a reason for hiding this comment

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

Responding to comments...

proposals/p0339.md Show resolved Hide resolved
proposals/p0339.md Outdated Show resolved Hide resolved
proposals/p0339.md Outdated Show resolved Hide resolved
proposals/p0339.md Outdated Show resolved Hide resolved
proposals/p0339.md Show resolved Hide resolved
proposals/p0339.md Show resolved Hide resolved
@jonmeow jonmeow changed the title var statement var <type> <identifier> [ = <value> ]; syntax for variables Apr 13, 2021
@jonmeow jonmeow changed the title var <type> <identifier> [ = <value> ]; syntax for variables Add var <type> <identifier> [ = <value> ]; syntax for variables Apr 13, 2021
zygoloid added a commit that referenced this pull request Apr 19, 2021
@jonmeow jonmeow requested a review from a team as a code owner April 20, 2021 15:50
@zygoloid zygoloid self-assigned this Apr 20, 2021
@jonmeow jonmeow merged commit 1f35541 into carbon-language:trunk Apr 28, 2021
@jonmeow jonmeow deleted the proposal-var-statement branch April 28, 2021 20:46
@github-actions github-actions bot added proposal accepted Decision made, proposal accepted and removed proposal rfc Proposal with request-for-comment sent out labels Apr 28, 2021
jonmeow added a commit that referenced this pull request May 17, 2021
Starting to apply #339

Co-authored-by: Richard Smith <[email protected]>
@jonmeow jonmeow mentioned this pull request Jul 2, 2021
jonmeow added a commit that referenced this pull request Jul 9, 2021
Propose the decision from #542, noting implementation from #563

Also integrates some of #339 into `variables.md` because that's actually how this started, looking for a proposal reference for #542 

Co-authored-by: Richard Smith <[email protected]>
Co-authored-by: josh11b <[email protected]>
chandlerc pushed a commit that referenced this pull request Jun 28, 2022
chandlerc pushed a commit that referenced this pull request Jun 28, 2022
chandlerc pushed a commit that referenced this pull request Jun 28, 2022
Starting to apply #339

Co-authored-by: Richard Smith <[email protected]>
chandlerc pushed a commit that referenced this pull request Jun 28, 2022
Propose the decision from #542, noting implementation from #563

Also integrates some of #339 into `variables.md` because that's actually how this started, looking for a proposal reference for #542 

Co-authored-by: Richard Smith <[email protected]>
Co-authored-by: josh11b <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes PR meets CLA requirements according to bot. proposal accepted Decision made, proposal accepted proposal A proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants