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

Explicitly assign constructed C++ classes #122062

Merged
merged 1 commit into from
Mar 7, 2024

Conversation

workingjubilee
Copy link
Member

C++ style guides I am aware of recommend specifically preferring = syntax for any classes with fairly obvious constructors1 that do not perform any complicated logic in their constructor. I contend that all constructors that the rustc_llvm code uses qualify. This has only become more common since C++ 17 guaranteed many cases of copy initialization elision.

The other detail is that I tried to ask another contributor with infinitely more C++ experience than me (i.e. any) what this constructor syntax was, and they thought it was a macro. I know of no other language that has adopted this same syntax. As the rustc codebase features many contributors experienced in many other languages, using a less... unique... style has many other benefits in making this code more lucid and maintainable, which is something it direly needs.

Footnotes

  1. e.g. https://abseil.io/tips/88

@rustbot
Copy link
Collaborator

rustbot commented Mar 6, 2024

r? @cuviper

rustbot has assigned @cuviper.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 6, 2024
@rust-log-analyzer

This comment has been minimized.

C++ style guides I am aware of recommend specifically preferring = syntax
for any classes with fairly obvious constructors[^0] that do not perform
any complicated logic in their constructor. I contend that all constructors
that the `rustc_llvm` code uses qualify. This has only become more common
since C++ 17 guaranteed many cases of copy initialization elision.

The other detail is that I tried to ask another contributor with
infinitely more C++ experience than me (i.e. any) what this constructor
syntax was, and they thought it was a macro. I know of no other language
that has adopted this same syntax. As the rustc codebase features many
contributors experienced in many other languages, using a less...
unique... style has many other benefits in making this code more
lucid and maintainable, which is something it direly needs.

[^0]: e.g. https://abseil.io/tips/88
@cuviper
Copy link
Member

cuviper commented Mar 6, 2024

Changing to the auto Name = Type(args...); style of initialization is ok with me, especially to avoid the "most vexing parse" of the traditional constructor syntax.

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Mar 6, 2024

📌 Commit 23623a0 has been approved by cuviper

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 6, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 7, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#122015 (Add better explanation for `rustc_index::IndexVec`)
 - rust-lang#122061 (Clarify FatalErrorHandler)
 - rust-lang#122062 (Explicitly assign constructed C++ classes)
 - rust-lang#122072 (Refer to "slice" instead of "vector" in Ord and PartialOrd trait impl of slices)
 - rust-lang#122088 (Remove unnecessary fixme on new thread stack size)
 - rust-lang#122094 (Remove outdated footnote "missing-stack-probe" in platform-support)
 - rust-lang#122107 (Temporarily make allow-by-default the `non_local_definitions` lint)
 - rust-lang#122109 (compiletest: Add a `//@ needs-threads` directive)

Failed merges:

 - rust-lang#122104 (Rust is a proper name: rust → Rust)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 869529a into rust-lang:master Mar 7, 2024
11 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Mar 7, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 7, 2024
Rollup merge of rust-lang#122062 - workingjubilee:initialize-my-fist, r=cuviper

Explicitly assign constructed C++ classes

C++ style guides I am aware of recommend specifically preferring = syntax for any classes with fairly obvious constructors[^0] that do not perform any complicated logic in their constructor. I contend that all constructors that the `rustc_llvm` code uses qualify. This has only become more common since C++ 17 guaranteed many cases of copy initialization elision.

The other detail is that I tried to ask another contributor with infinitely more C++ experience than me (i.e. any) what this constructor syntax was, and they thought it was a macro. I know of no other language that has adopted this same syntax. As the rustc codebase features many contributors experienced in many other languages, using a less... unique... style has many other benefits in making this code more lucid and maintainable, which is something it direly needs.

[^0]: e.g. https://abseil.io/tips/88
@workingjubilee workingjubilee deleted the initialize-my-fist branch March 7, 2024 03:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants