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

Unused Pattern Bindings (Unused Function Parameters) #2022

Merged

Conversation

matthew-russo
Copy link
Contributor

@matthew-russo matthew-russo commented Aug 13, 2022

This proposal specifies how unused pattern bindings are written in the Carbon programming language. This is a more general problem statement of "how do users specify unused function parameters" as function parameter declarations are a more specific form of pattern

Related issue #1996

@matthew-russo matthew-russo force-pushed the proposal-optional-argument-na branch from 49fd1ad to 5e2b309 Compare August 13, 2022 04:29
@matthew-russo matthew-russo changed the title Optional Argument Names (Unused Arguments) Unused Pattern Bindings (Unused Function Parameters) Aug 13, 2022
@matthew-russo matthew-russo force-pushed the proposal-optional-argument-na branch 4 times, most recently from eb518d5 to 966746a Compare August 13, 2022 17:44
@matthew-russo matthew-russo marked this pull request as ready for review August 13, 2022 17:46
@matthew-russo
Copy link
Contributor Author

I framed this around patterns more generally based on the discussion in #476. If this should only talk about function parameters, let me know and I'll adjust the wording

Copy link
Contributor

@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.

Thank you! This proposal looks pretty good to me. There are just a couple aspects I see which I think it may be helpful to provide info on (or specifically mark as open questions not resolved here, so that people don't accidentally read this as resolving them).

proposals/p2022.md Show resolved Hide resolved
proposals/p2022.md Show resolved Hide resolved
proposals/p2022.md Show resolved Hide resolved
@matthew-russo matthew-russo force-pushed the proposal-optional-argument-na branch from 966746a to 8737e61 Compare August 20, 2022 04:56
@matthew-russo
Copy link
Contributor Author

matthew-russo commented Aug 20, 2022

Sorry for the delay in updating this. I've pushed some small changes that cover the areas you suggested -- they're very high level, let me know if I need to go in to more detail on context or the why behind them and I can spend a bit more time with it.

@jonmeow jonmeow added proposal A proposal proposal rfc Proposal with request-for-comment sent out labels Aug 24, 2022
@jonmeow
Copy link
Contributor

jonmeow commented Aug 24, 2022

I see this had been missing proposal labels -- I've added those to help make sure it's getting reviewed as such (leads have been busy but I think there'll be increased availability now)

Copy link
Contributor

@josh11b josh11b 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 filling in for @jonmeow since he's on vacation.

proposals/p2022.md Outdated Show resolved Hide resolved
proposals/p2022.md Outdated Show resolved Hide resolved
proposals/p2022.md Outdated Show resolved Hide resolved
proposals/p2022.md Outdated Show resolved Hide resolved
proposals/p2022.md Outdated Show resolved Hide resolved
proposals/p2022.md Outdated Show resolved Hide resolved
proposals/p2022.md Outdated Show resolved Hide resolved
proposals/p2022.md Outdated Show resolved Hide resolved
proposals/p2022.md Outdated Show resolved Hide resolved
proposals/p2022.md Outdated Show resolved Hide resolved
@matthew-russo matthew-russo force-pushed the proposal-optional-argument-na branch from 8737e61 to 479c79f Compare September 5, 2022 20:54
proposals/p2022.md Outdated Show resolved Hide resolved
proposals/p2022.md Outdated Show resolved Hide resolved
@jonmeow
Copy link
Contributor

jonmeow commented Sep 7, 2022

Looks good to me, noting outstanding comments.

Just a tip regarding force-pushes though -- those can break GitHub's comment associations, and it makes it harder to determine what's changed since the last review (with regular pushes, GitHub can show a delta). We're going to squash-and-merge PRs anyways, so if you use regular pushes the end result will actually be the same and it can make it easier to review. :)

@matthew-russo
Copy link
Contributor Author

Looks good to me, noting outstanding comments.

Just a tip regarding force-pushes though -- those can break GitHub's comment associations, and it makes it harder to determine what's changed since the last review (with regular pushes, GitHub can show a delta). We're going to squash-and-merge PRs anyways, so if you use regular pushes the end result will actually be the same and it can make it easier to review. :)

Whoops sorry about that -- will adjust going forward

Copy link
Contributor

@geoffromer geoffromer left a comment

Choose a reason for hiding this comment

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

Looks good to me.

proposals/p2022.md Outdated Show resolved Hide resolved
proposals/p2022.md Outdated Show resolved Hide resolved
The inclusion of two syntaxes allows authors to decide when they will favor
conciseness or explicitness over the other.

## Alternatives considered
Copy link
Contributor

Choose a reason for hiding this comment

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

Good call, suggested more specific wording below.

josh11b added a commit that referenced this pull request Sep 13, 2022
Includes the advice from #2022 (comment)

Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
Copy link
Contributor

@zygoloid zygoloid left a comment

Choose a reason for hiding this comment

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

Looks good to me.

proposals/p2022.md Outdated Show resolved Hide resolved
proposals/p2022.md Show resolved Hide resolved
Copy link
Contributor

@zygoloid zygoloid left a comment

Choose a reason for hiding this comment

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

LGTM

proposals/p2022.md Outdated Show resolved Hide resolved
zygoloid added a commit to zygoloid/carbon-lang that referenced this pull request Sep 20, 2022
Copy link
Contributor

@zygoloid zygoloid left a comment

Choose a reason for hiding this comment

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

Approving on behalf of carbon leads. Thank you!

One concern during leads discussion was how often unused is used as an identifier in C++ code. We investigated and found that most uses would actually be either _ or unused foo: ... in Carbon, and there were few enough remaining cases that we were OK with taking unused as a keyword.

proposals/p2022.md Outdated Show resolved Hide resolved
proposals/p2022.md Outdated Show resolved Hide resolved
@zygoloid zygoloid merged commit b464a0a into carbon-language:trunk Sep 21, 2022
@zygoloid zygoloid added proposal accepted Decision made, proposal accepted and removed proposal rfc Proposal with request-for-comment sent out labels Sep 21, 2022
zygoloid added a commit to zygoloid/carbon-lang that referenced this pull request Sep 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal accepted Decision made, proposal accepted proposal A proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants