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

Convert CommandLine member references to pointers #4301

Merged
merged 4 commits into from
Sep 20, 2024

Conversation

jonmeow
Copy link
Contributor

@jonmeow jonmeow commented Sep 11, 2024

This follows up on a style question about whether to prefer reference members or pointers. This PR converts to pointers as a demonstration of that style choice.

Note, I'm trying to update constructors to match use of * based on whether they keep a reference. I'm removing a few const& uses where no reference was kept (i.e., it was just copied, and didn't seem worth a move).

I'm changing AddArgImpl to return an Arg* because it just gets passed to a constructor, seems simpler this way.

@jonmeow jonmeow requested a review from josh11b September 13, 2024 17:51
@jonmeow
Copy link
Contributor Author

jonmeow commented Sep 13, 2024

Discussion seems to be leaning towards more passing by reference, so I've changed that here.

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.

Approving since this matches what was discussed in #style, even though I prefer the all pointer version.

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.

Just wanted to say -- this also LGTM as is as an incremental step. e can fix forward with whatever subsequent style tweaks come up. Or happy if you'd rather update this PR based on the latest discussions.

@jonmeow
Copy link
Contributor Author

jonmeow commented Sep 20, 2024

That's fine, given the prior commit already had pointers I think it's easier to unroll to that here. I've also add "must not be null" comments per your style request.

@josh11b josh11b added this pull request to the merge queue Sep 20, 2024
Merged via the queue into carbon-language:trunk with commit 9b0519d Sep 20, 2024
8 checks passed
@jonmeow jonmeow deleted the commandline branch September 20, 2024 21:31
github-merge-queue bot pushed a commit that referenced this pull request Sep 20, 2024
[Context](https://discord.com/channels/655572317891461132/821113559755784242/1283516297686286377).
Some relevant Google C++ style is at [Inputs and
Outputs](https://google.github.io/styleguide/cppguide.html#Inputs_and_Outputs).
#4301 is an example application of the style.

---------

Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants