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

Proposal: Property naming in C++ #720

Merged
merged 11 commits into from
Sep 28, 2021
Merged

Proposal: Property naming in C++ #720

merged 11 commits into from
Sep 28, 2021

Conversation

geoffromer
Copy link
Contributor

@geoffromer geoffromer commented Aug 7, 2021

This proposed style change allows C++ classes in the Carbon project to provide methods that are named like variables, so long as they behave like properties of the class. It also requires data member names to have a trailing _.

@google-cla google-cla bot added the cla: yes PR meets CLA requirements according to bot. label Aug 7, 2021
@geoffromer geoffromer marked this pull request as ready for review August 7, 2021 00:26
@geoffromer geoffromer requested review from a team as code owners August 7, 2021 00:26
@geoffromer geoffromer added this to Draft in Proposals via automation Aug 7, 2021
@geoffromer geoffromer moved this from Draft to RFC in Proposals Aug 7, 2021
@github-actions github-actions bot added proposal A proposal proposal rfc Proposal with request-for-comment sent out labels Aug 7, 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.

Nice! I like the wording in the style change.

I think if anything, the proposal has more motivation and detail than it needs, but that's not really a bad thing. =D I've illustrated a slightly different way of arriving at the same conclusion below mostly because it seemed interestingly different. If it happens to give a way to shorten and focus the motivation more, great, but really don't spend time worrying about it as I think what you have is also good. =]

Will at least let a couple of other eyes see this before fully stamping it, but I'm generally quite happy.

docs/project/cpp_style_guide.md Outdated Show resolved Hide resolved
proposals/new.md Outdated Show resolved Hide resolved
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.

It might be worth noting somewhere, this'll also mean clang-tidy can't really re-case function names. Which perhaps should be included in this PR (a .clang-tidy update).

proposals/new.md Outdated Show resolved Hide resolved
docs/project/cpp_style_guide.md Outdated Show resolved Hide resolved
docs/project/cpp_style_guide.md Outdated Show resolved Hide resolved
docs/project/cpp_style_guide.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, but check with Jon about the outstanding comments. I think the new wording works, but good to confirm.

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.

I've been thinking about this, and maybe the omission of setter methods means I disagree with this proposal. It's not that I disagree with the accessor naming, it's that I disagree with the argument against mutators.

I can talk about this at length, but I view getters and setters as inseparable. There are plenty of examples in C++: in our underlying style guide, C++ tutorials, and elsewhere. To the extent that this proposal concludes my_var() + SetMyVar(x), I view this proposal as leaving code in an uncanny valley.

I can't see Carbon implementing getters without setters (I think there's no cross-language precedent for having one without the other), and I think there enough examples in other languages that I think MyObj.my_prop = x; syntax should be expected. Given that precedent, I think having getters without setters in our C++ style is a similar inconsistency that shouldn't be adopted.

docs/project/cpp_style_guide.md Outdated Show resolved Hide resolved
@geoffromer
Copy link
Contributor Author

It might be worth noting somewhere, this'll also mean clang-tidy can't really re-case function names. Which perhaps should be included in this PR (a .clang-tidy update).

Done.

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.

I think the next step here is just to update for mutating?

proposals/p0720.md Outdated Show resolved Hide resolved
@jonmeow jonmeow mentioned this pull request Sep 21, 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.

I'm generally happy here, a minor suggestion below. I'd like to check with @zygoloid before you land it though.

proposals/p0720.md Outdated Show resolved Hide resolved
geoffromer and others added 2 commits September 22, 2021 12:09
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
@jonmeow
Copy link
Contributor

jonmeow commented Sep 27, 2021

I'm generally happy here, a minor suggestion below. I'd like to check with @zygoloid before you land it though.

@chandlerc @zygoloid Any updates here?

@chandlerc
Copy link
Contributor

I'm generally happy here, a minor suggestion below. I'd like to check with @zygoloid before you land it though.

@chandlerc @zygoloid Any updates here?

Chatted, and I think we're all fine.

@geoffromer geoffromer merged commit 97ba7b8 into carbon-language:trunk Sep 28, 2021
Proposals automation moved this from RFC to Accepted Sep 28, 2021
@geoffromer geoffromer deleted the property-style branch September 28, 2021 21:36
@github-actions github-actions bot added proposal accepted Decision made, proposal accepted and removed proposal rfc Proposal with request-for-comment sent out labels Sep 28, 2021
jonmeow added a commit that referenced this pull request Oct 19, 2021
This is the last PR I plan to have focused on #720
@jonmeow jonmeow mentioned this pull request Jan 7, 2022
chandlerc added a commit that referenced this pull request Jun 28, 2022
This proposed style change allows C++ classes in the Carbon project to provide methods that are named like variables, so long as they behave like _properties_ of the class. It also requires data member names to have a trailing `_`.

Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
chandlerc pushed a commit that referenced this pull request Jun 28, 2022
This is the last PR I plan to have focused on #720
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

3 participants