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

if/else #285

Merged
merged 17 commits into from
Mar 24, 2021
Merged

if/else #285

merged 17 commits into from
Mar 24, 2021

Conversation

jonmeow
Copy link
Contributor

@jonmeow jonmeow commented Feb 23, 2021

No description provided.

@jonmeow jonmeow added WIP proposal A proposal labels Feb 23, 2021
@jonmeow
Copy link
Contributor Author

jonmeow commented Feb 23, 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 Feb 23, 2021

## Proposal

We should make `if`/`else` syntax basically consistent with C/C++, rather than
Copy link

Choose a reason for hiding this comment

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

I find "basically" to confuse this point. What is if anything is different from C++?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing "basically", it's not a great word to add anyways.

@zygoloid
Copy link
Contributor

I have no concerns. LGTM!

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.

Should this also change the "provisional" text in the language overview?

proposals/p0285.md Outdated Show resolved Hide resolved
proposals/p0285.md Outdated Show resolved Hide resolved
Co-authored-by: josh11b <josh11b@users.noreply.github.com>
@jonmeow
Copy link
Contributor Author

jonmeow commented Feb 24, 2021

Should this also change the "provisional" text in the language overview?

This is just a proposal doc, and doesn't include design changes. Those would be handled separately, through code review.

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.

This looks good.

Small request: could you add a few complete test programs and expected results (for addition to the testdata/ directory of the executable semantics).

@jonmeow
Copy link
Contributor Author

jonmeow commented Feb 24, 2021

Sorry, but I'd really rather not block this on changes to executable semantics. That is not a requirement, and honestly I'd rather just move forward (with the proposal).

@jonmeow jonmeow added proposal rfc Proposal with request-for-comment sent out and removed WIP labels Feb 24, 2021
@jsiek
Copy link
Contributor

jsiek commented Feb 25, 2021

Hi Jon, My comment wasn't intended to block this... just meant as encouragement to collaborate with me on the executable semantics in parallel. I've added three test cases this afternoon... you might take a look to see if you think I've got the corner cases covered.

@chandlerc
Copy link
Contributor

LGTM for what it's worth.

I'd personally probably go one step further and require {}s and support basically:

if (<expression>) { ... } [ else if (<expression>) { ... } ][ else { ... } ]

And none of the braces are optional.

This is a strict subset of C++, with no further features. But it has no ambiguous else or goto-fail concerns or optional braces.

That said, I am 100% fine with this proposal going in, and suggesting the above as a follow-up to restrict things unless Jon and others specifically want to narrow to this subset in this early proposal.

@dabrahams
Copy link

dabrahams commented Feb 26, 2021

Not a comment on the proposal, which is fine. Just to note in response to @chandlerc, if the braces are required, there's no need to require the parens. One nice effect that has is that C++ programmers' old instincts to put the parens in still creates legal code.

@chandlerc
Copy link
Contributor

Not a comment on the proposal, which is fine. Just to note in response to @chandlerc, if the braces are required, there's no need to require the parens.

Definitely an option, but not one I'd even want to consider here as that goes beyond simple sunsetting of C++. I'd suggest that and other extension like expression usage be later proposals.

@jonmeow
Copy link
Contributor Author

jonmeow commented Feb 26, 2021

@dabrahams Regarding parentheses, please note the third bullet in the alternative:

  • Parentheses allow introducing syntactic variants in the future without
    ambiguity.
    • For example, C++'s if constexpr (...) wouldn't have been possible if
      the parentheses were optional.

(even if braces are required, this issue still applies, and thus parens should not be assumed to not be required)

Also, please be careful not to @ people who are not part of Carbon (@code).

@dabrahams
Copy link

@jonmeow could you explain where the constexpr ambiguity would come from when braces are required? That doesn't seem obvious to me.

Naturally @​code was a typo. I will endeavor in future only to type the things I mean to type.

@jonmeow
Copy link
Contributor Author

jonmeow commented Feb 26, 2021

Consider the parentheses-less code if constexpr int i = id(101) { } (borrowing the example from https://github.com/llvm/llvm-project/blob/main/clang/test/CXX/drs/dr9xx.cpp#L64).

That can be interpreted as either if (constexpr int i = id(101)) { } or if constexpr (int i = id(101)) { }. Note the resulting placement of parentheses differs.

An interesting, possibly more complex case of this is:

if (constexpr absl::string_view kFoo = "foo"; absl::StartsWith(arg, kFoo)) {

In either case, it's actually the difference between whether constexpr binds with what's on the left or the right. The braces requirement does not affect the ability to make a choice, only the parentheses.

jonmeow and others added 2 commits February 26, 2021 13:50
Co-authored-by: Geoff Romer <gromer@google.com>
@mmdriley
Copy link
Contributor

mmdriley commented Mar 2, 2021

Comment deadline set for March 9 -- ref https://forums.carbon-lang.dev/t/rfc-if-else-285/215/2

@mmdriley mmdriley added comment deadline and removed proposal rfc Proposal with request-for-comment sent out comment deadline labels Mar 9, 2021
@jonmeow jonmeow added this to All proposals in Proposals Mar 17, 2021
@jonmeow jonmeow moved this from All proposals to Decision pending in Proposals Mar 17, 2021
@jonmeow jonmeow moved this from Needs decision to All proposals in Proposals Mar 17, 2021
@jonmeow jonmeow added this to the Proposal Done milestone Mar 17, 2021
@mmdriley mmdriley mentioned this pull request Mar 22, 2021
@mmdriley mmdriley added proposal accepted Decision made, proposal accepted and removed needs decision labels Mar 22, 2021
@mmdriley
Copy link
Contributor

@jonmeow you're good to resolve and merge here

@jonmeow jonmeow moved this from Decision pending to Accepted in Proposals Mar 24, 2021
@jonmeow jonmeow requested review from a team and mmdriley March 24, 2021 22:30
@jonmeow
Copy link
Contributor Author

jonmeow commented Mar 24, 2021

@mmdriley Can you approve for commit? (proposals need a review manager's approval)

Copy link
Contributor

@mmdriley mmdriley left a comment

Choose a reason for hiding this comment

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

Ack! Sorry I was blocking this.

@jonmeow jonmeow merged commit a20ae0f into carbon-language:trunk Mar 24, 2021
@jonmeow jonmeow deleted the proposal-if/else branch March 24, 2021 23:02
zygoloid added a commit that referenced this pull request Apr 20, 2021
chandlerc pushed a commit that referenced this pull request Jun 28, 2022
chandlerc pushed a commit that referenced this pull request Jun 28, 2022
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
No open projects
Proposals
Accepted
Development

Successfully merging this pull request may close these issues.

None yet

9 participants