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

Require braces #623

Merged
merged 19 commits into from
Jul 22, 2021
Merged

Require braces #623

merged 19 commits into from
Jul 22, 2021

Conversation

jonmeow
Copy link
Contributor

@jonmeow jonmeow commented Jul 2, 2021

Require braces, never optional, particularly in control flow like if/else.

@jonmeow jonmeow added the proposal A proposal label Jul 2, 2021
@jonmeow jonmeow requested a review from a team July 2, 2021 22:47
@google-cla google-cla bot added the cla: yes PR meets CLA requirements according to bot. label Jul 2, 2021
@jonmeow jonmeow marked this pull request as ready for review July 7, 2021 17:06
@github-actions github-actions bot added the proposal rfc Proposal with request-for-comment sent out label Jul 7, 2021
@jonmeow jonmeow changed the title require-braces Require braces Jul 7, 2021
@jonmeow jonmeow marked this pull request as draft July 7, 2021 17:09
@jonmeow jonmeow marked this pull request as ready for review July 7, 2021 17:45
@chandlerc chandlerc self-assigned this Jul 7, 2021
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.

Thanks, this generally looks good. I don't think I had any major comments.

proposals/p0623.md Outdated Show resolved Hide resolved
proposals/p0623.md Outdated Show resolved Hide resolved
proposals/p0623.md Outdated Show resolved Hide resolved
proposals/p0623.md Outdated Show resolved Hide resolved
proposals/p0623.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/p0623.md Show resolved Hide resolved
proposals/p0623.md Outdated Show resolved Hide resolved
proposals/p0623.md Show resolved Hide resolved
proposals/p0623.md Outdated Show resolved Hide resolved
proposals/p0623.md Show resolved Hide resolved
proposals/p0623.md Outdated Show resolved Hide resolved
proposals/p0623.md Show resolved Hide resolved
jonmeow and others added 3 commits July 9, 2021 11:42
@jonmeow
Copy link
Contributor Author

jonmeow commented Jul 9, 2021

(comment pass, responded on topic but forgot to do a review)

@jonmeow
Copy link
Contributor Author

jonmeow commented Jul 15, 2021

I was moving #655 to draft and remembered else if, so I've added a note calling that out here as correct code.

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.

Really sorry about the delay here... I had several comments and apparently didn't mash the "send" button. =[[[ Hopefully all easy changes just adding information. I've tried to make it very clear what I tihnk is useful to add, and I'm not expecting this to change the outcome of anything.

Comment on lines 45 to 47
Some modern languages require braces, including Go, Rust, and Swift. Kotlin is
an example which does not. Note some which require braces make parentheses
optional.
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems worth including as background the CERT rule:
https://wiki.sei.cmu.edu/confluence/display/c/EXP19-C.+Use+braces+for+the+body+of+an+if%2C+for%2C+or+while+statement

And the blog post about goto fail;: https://www.imperialviolet.org/2014/02/22/applebug.html

FWIW, I'm not suggesting that goto fail; should be a strong rationale for this pattern, but I do think it should be addressed explicitly as it is often cited as a reason for this change. FWIW, I agree with @zygoloid below that simplifying the language grammar seems like a strong motivation. I actually like the summary of the importance of braces here in the blog post:

Maybe the coding style contributed to this by allowing ifs without braces, but one can have incorrect indentation with braces too, so that doesn't seem terribly convincing to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems worth including as background the CERT rule:
https://wiki.sei.cmu.edu/confluence/display/c/EXP19-C.+Use+braces+for+the+body+of+an+if%2C+for%2C+or+while+statement

Linked.

And the blog post about goto fail;: https://www.imperialviolet.org/2014/02/22/applebug.html

Added, moved up the link that was in rationale.

FWIW, I'm not suggesting that goto fail; should be a strong rationale for this pattern, but I do think it should be addressed explicitly as it is often cited as a reason for this change. FWIW, I agree with @zygoloid below that simplifying the language grammar seems like a strong motivation. I actually like the summary of the importance of braces here in the blog post:

Maybe the coding style contributed to this by allowing ifs without braces, but one can have incorrect indentation with braces too, so that doesn't seem terribly convincing to me.

Are you saying to remove the mention of goto fail from the rationale? Do you want this sentence somewhere in particular?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nah, I'm happy mentioning it, I just didn't want to overstate its importance. I've suggested a place where it could even be included in the disadvantages section, but that's optional given the comment in the rationale. Up to you.

proposals/p0623.md Show resolved Hide resolved
proposals/p0623.md Show resolved Hide resolved
proposals/p0623.md Show resolved Hide resolved
proposals/p0623.md Outdated 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.

Addressing comments

proposals/p0623.md Show resolved Hide resolved
proposals/p0623.md Show resolved Hide resolved
proposals/p0623.md Outdated Show resolved Hide resolved
Comment on lines 45 to 47
Some modern languages require braces, including Go, Rust, and Swift. Kotlin is
an example which does not. Note some which require braces make parentheses
optional.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems worth including as background the CERT rule:
https://wiki.sei.cmu.edu/confluence/display/c/EXP19-C.+Use+braces+for+the+body+of+an+if%2C+for%2C+or+while+statement

Linked.

And the blog post about goto fail;: https://www.imperialviolet.org/2014/02/22/applebug.html

Added, moved up the link that was in rationale.

FWIW, I'm not suggesting that goto fail; should be a strong rationale for this pattern, but I do think it should be addressed explicitly as it is often cited as a reason for this change. FWIW, I agree with @zygoloid below that simplifying the language grammar seems like a strong motivation. I actually like the summary of the importance of braces here in the blog post:

Maybe the coding style contributed to this by allowing ifs without braces, but one can have incorrect indentation with braces too, so that doesn't seem terribly convincing to me.

Are you saying to remove the mention of goto fail from the rationale? Do you want this sentence somewhere in particular?

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 and thanks! I confirmed w/ the leads they're happy with this direction too so we're good to go. Thanks so much for getting it clearly written up!

proposals/p0623.md Outdated Show resolved Hide resolved
Comment on lines 45 to 47
Some modern languages require braces, including Go, Rust, and Swift. Kotlin is
an example which does not. Note some which require braces make parentheses
optional.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nah, I'm happy mentioning it, I just didn't want to overstate its importance. I've suggested a place where it could even be included in the disadvantages section, but that's optional given the comment in the rationale. Up to you.

proposals/p0623.md Show resolved Hide resolved
@jonmeow jonmeow merged commit cab833e into carbon-language:trunk Jul 22, 2021
@jonmeow jonmeow deleted the proposal-require-braces branch July 22, 2021 17:42
@github-actions github-actions bot added proposal accepted Decision made, proposal accepted and removed proposal rfc Proposal with request-for-comment sent out labels Jul 22, 2021
jonmeow added a commit that referenced this pull request Jul 28, 2021
zygoloid added a commit that referenced this pull request Jul 31, 2021
For error recovery, allow the braces around the controlled statement of
an `if`, `while`, or similar to be omitted. Remove support for nested
code blocks as this conflicts with struct literal syntax.
chandlerc added a commit that referenced this pull request Jun 28, 2022
Require braces, never optional, particularly in control flow like `if`/`else`.

Co-authored-by: Richard Smith <[email protected]>
Co-authored-by: Chandler Carruth <[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
For error recovery, allow the braces around the controlled statement of
an `if`, `while`, or similar to be omitted. Remove support for nested
code blocks as this conflicts with struct literal syntax.
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