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

auto keyword for vars #851

Merged
merged 14 commits into from
Jan 10, 2022
Merged

auto keyword for vars #851

merged 14 commits into from
Jan 10, 2022

Conversation

jonmeow
Copy link
Contributor

@jonmeow jonmeow commented Sep 24, 2021

Allow var <identifier>: auto = <expression>; syntax.

@jonmeow jonmeow added the proposal A proposal label Sep 24, 2021
@jonmeow jonmeow requested a review from a team September 24, 2021 16:37
@google-cla google-cla bot added the cla: yes PR meets CLA requirements according to bot. label Sep 24, 2021
@jonmeow jonmeow changed the title var-auto auto keyword for vars Sep 24, 2021
@jonmeow jonmeow marked this pull request as ready for review September 27, 2021 19:01
@github-actions github-actions bot added the proposal rfc Proposal with request-for-comment sent out label Sep 27, 2021
Copy link
Contributor

@chandlerc chandlerc left a comment

Choose a reason for hiding this comment

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

Sorry that this proposal got largely missed and not promptly reviewed.

I generally like the direction here. I've left one comment, but its pretty minor. I do want to check that the @carbon-language/carbon-leads don't want to see anything else here before moving forward or don't have any concerns.

Comment on lines +308 to +313
Disadvantages:

- Type inference becomes the _lack_ of a type, rather than the presence of
something different.
- C++'s syntax legacy may have needed `auto` in order to provide type
inference, where Carbon does not.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it'd be good to also capture that this creates the potential for ambiguity with expression patterns as well. There are possible ways of addressing that ambiguity (some of which you get into in the background section), but I feel like its still an important consideration in the tradeoff here. @zygoloid might even be able to suggest something here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a bullet for that, linking the background.

Copy link
Contributor

Choose a reason for hiding this comment

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

Mid-air collision between your update and my suggestion. Here's what I'd written up (which looks similar enough to your new bullet):

Suggested change
Disadvantages:
- Type inference becomes the _lack_ of a type, rather than the presence of
something different.
- C++'s syntax legacy may have needed `auto` in order to provide type
inference, where Carbon does not.
Disadvantages:
- Type inference becomes the _lack_ of a type, rather than the presence of
something different.
- C++'s syntax legacy may have needed `auto` in order to provide type
inference, where Carbon does not.
- Removes syntactic distinction between binding of a new name and reference
to an existing name. Reintroducing this distinction would likely require
additional syntax, such as Swift's nested `let`.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noting here -- replaced mine with @zygoloid's

proposals/p0851.md Outdated Show resolved Hide resolved
Comment on lines +308 to +313
Disadvantages:

- Type inference becomes the _lack_ of a type, rather than the presence of
something different.
- C++'s syntax legacy may have needed `auto` in order to provide type
inference, where Carbon does not.
Copy link
Contributor

Choose a reason for hiding this comment

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

Mid-air collision between your update and my suggestion. Here's what I'd written up (which looks similar enough to your new bullet):

Suggested change
Disadvantages:
- Type inference becomes the _lack_ of a type, rather than the presence of
something different.
- C++'s syntax legacy may have needed `auto` in order to provide type
inference, where Carbon does not.
Disadvantages:
- Type inference becomes the _lack_ of a type, rather than the presence of
something different.
- C++'s syntax legacy may have needed `auto` in order to provide type
inference, where Carbon does not.
- Removes syntactic distinction between binding of a new name and reference
to an existing name. Reintroducing this distinction would likely require
additional syntax, such as Swift's nested `let`.

proposals/p0851.md Outdated Show resolved Hide resolved
@jonmeow jonmeow requested review from chandlerc and zygoloid January 7, 2022 22:49
proposals/p0851.md Outdated Show resolved Hide resolved
Copy link
Contributor

@chandlerc chandlerc left a comment

Choose a reason for hiding this comment

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

LGTM, this looks really nice. =D

@jonmeow jonmeow merged commit 78ced89 into carbon-language:trunk Jan 10, 2022
@jonmeow jonmeow deleted the proposal-var-auto branch January 10, 2022 18:08
@github-actions github-actions bot added proposal accepted Decision made, proposal accepted and removed proposal rfc Proposal with request-for-comment sent out labels Jan 10, 2022
jonmeow added a commit that referenced this pull request Jan 28, 2022
I was doing this for #851 initially, but I think #438 and #826 hadn't made it in (possibly intentionally due to #851? I don't recall). 

Co-authored-by: Chandler Carruth <[email protected]>
Co-authored-by: Richard Smith <[email protected]>
chandlerc pushed a commit that referenced this pull request Jun 28, 2022
Allow `var <identifier>: auto = <expression>;` syntax.

Co-authored-by: Richard Smith <[email protected]>

Co-authored-by: josh11b <[email protected]>
chandlerc added a commit that referenced this pull request Jun 28, 2022
I was doing this for #851 initially, but I think #438 and #826 hadn't made it in (possibly intentionally due to #851? I don't recall). 

Co-authored-by: Chandler Carruth <[email protected]>
Co-authored-by: Richard Smith <[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.

4 participants