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

[WIP] Massive cosmetic PR, take 2 #58036

Closed
wants to merge 15 commits into from
Closed

Conversation

alexreg
Copy link
Contributor

@alexreg alexreg commented Jan 31, 2019

This is my second take on #57016, where I am doing a few things differently:

  • Splitting into many commits, depending on area of codebase and type of change. (I shall be doing this over the coming days.)
  • Excluding more controversial changes from previous attempt.
  • Factoring off subsets of commits into separate PRs to get met reviewed and merged separately, as needed.

This is sort of like a "tracking PR" for now, and probably shouldn't be merged as-is unless someone is happy with all my changes.

CC @nikomatsakis @steveklabnik

@rust-highfive
Copy link
Collaborator

r? @pnkfelix

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 31, 2019
@rust-highfive

This comment has been minimized.

@pnkfelix
Copy link
Member

pnkfelix commented Feb 1, 2019

I'm going to put WIP in the title to reflect that this is "a 'tracking PR' for now" that shoud not be merged as is.

@pnkfelix pnkfelix changed the title Massive cosmetic PR, take 2 [WIP] Massive cosmetic PR, take 2 Feb 1, 2019
@bors
Copy link
Contributor

bors commented Feb 1, 2019

☔ The latest upstream changes (presumably #57916) made this pull request unmergeable. Please resolve the merge conflicts.

@alexreg
Copy link
Contributor Author

alexreg commented Feb 1, 2019

@pnkfelix Sure. I’ll try to finish the commit splitting later today. In the meanwhile, please let me if I can reorganise things any mire to help get this merged.

Copy link
Contributor

@oli-obk oli-obk 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 not quite clear what the rules are where comments start with a capitalized letter and where they start with a lower case letter. As far as I can see there's both happening.

I reviewed the first commit, but I might have missed some things

src/librustc/traits/auto_trait.rs Show resolved Hide resolved
src/librustc/traits/mod.rs Outdated Show resolved Hide resolved
src/librustc/traits/object_safety.rs Outdated Show resolved Hide resolved
src/librustc/traits/select.rs Outdated Show resolved Hide resolved
@@ -835,7 +835,7 @@ pub type CanonicalUserType<'gcx> = Canonical<'gcx, UserType<'gcx>>;

impl CanonicalUserType<'gcx> {
/// Returns `true` if this represents a substitution of the form `[?0, ?1, ?2]`,
/// i.e. each thing is mapped to a canonical variable with the same index.
/// i.e., each thing is mapped to a canonical variable with the same index.
Copy link
Contributor

Choose a reason for hiding this comment

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

these ., look really weird 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.

It's weird, but it's standard in U.S. English. :-) Not in British English. The American motivation is that you're abbreviating "id est, ...", which is Latin for "that is, ...". In that sense, it makes sense, but I know what you mean still.

@@ -186,7 +186,7 @@ impl Semantics for X87DoubleExtendedS {
/// exponent = all 1's, integer bit 0, significand 0 ("pseudoinfinity")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we're supposed to keep the librustc_apfloat crate (docs) in sync with llvm's. So the best course of action would be an additional patch against llvm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hard to tell... it looks like some docs have already been Rustified.

src/libsyntax/parse/parser.rs Outdated Show resolved Hide resolved
src/libsyntax/parse/parser.rs Show resolved Hide resolved
src/libsyntax/parse/parser.rs Show resolved Hide resolved
src/libsyntax/parse/parser.rs Outdated Show resolved Hide resolved
@rust-highfive

This comment has been minimized.

src/build_helper/lib.rs Outdated Show resolved Hide resolved
src/build_helper/lib.rs Outdated Show resolved Hide resolved
src/liballoc/collections/btree/set.rs Outdated Show resolved Hide resolved
src/liballoc/tests/lib.rs Outdated Show resolved Hide resolved
@rust-highfive

This comment has been minimized.

src/bootstrap/builder.rs Outdated Show resolved Hide resolved
src/liballoc/collections/btree/node.rs Show resolved Hide resolved
@bors
Copy link
Contributor

bors commented Feb 3, 2019

☔ The latest upstream changes (presumably #58079) made this pull request unmergeable. Please resolve the merge conflicts.

@@ -327,7 +327,7 @@ fn map_lib_features(base_src_path: &Path,
}
becoming_feature = None;
if line.contains("rustc_const_unstable(") {
// `const fn` features are handled specially.
// Const fn features are handled specially.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this is a good change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, happy to undo!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On second thought, I ran a search, and it seems "const fn" without the quotes is much more common in comments. I think it's okay personally, since "fn" can be considered an abbreviation for "function", and "const" and attribute/adjective.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(But I'll still revert it if you prefer.)

Copy link
Member

Choose a reason for hiding this comment

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

I'm not entirely sure that commonality is the right argument here. I don't think that either of these is particularly bad, but given that it's a specific rust feature, and that's it's name, const fn seems like its "proper name". Dunno.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might not be... I am just of the mind to opt for consistency when in doubt as to which is "morally" better. "Fn" is used as an abbreviation all over the codebase without backticks as well, it's worth saying. Will leave this for now, but just tell me to revert if you decide on that. Just pushed BTW, so feel free to continue reviewing. Thanks!

@alexreg
Copy link
Contributor Author

alexreg commented Feb 4, 2019

(About to push some changes, FYI. @steveklabnik)

@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Feb 5, 2019

☔ The latest upstream changes (presumably #57973) made this pull request unmergeable. Please resolve the merge conflicts.

@rust-highfive

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Feb 17, 2019

☔ The latest upstream changes (presumably #58495) made this pull request unmergeable. Please resolve the merge conflicts.

@petrochenkov petrochenkov added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 23, 2019
@petrochenkov
Copy link
Contributor

Blocked on #58619

@alexreg alexreg closed this Mar 1, 2019
gnzlbg pushed a commit to rust-lang/libtest that referenced this pull request Mar 2, 2019
Cosmetic improvements to doc comments

This has been factored out from rust-lang/rust#58036 to only include changes to documentation comments (throughout the rustc codebase).

r? @steveklabnik

Once you're happy with this, maybe we could get it through with r=1, so it doesn't constantly get invalidated? (I'm not sure this will be an issue, but just in case...) Anyway, thanks for your advice so far!
bors added a commit to rust-lang/rust-clippy that referenced this pull request Mar 10, 2019
Various cosmetic improvements

Related to the larger effort of rust-lang/rust#58036.
Xanewok added a commit to Xanewok/rls-vscode that referenced this pull request Mar 19, 2019
These are not legally required and are mostly noise. See:
* rust-lang/rust#58036
* rust-lang/rls#1326
Xanewok added a commit to rust-lang/rls that referenced this pull request Apr 20, 2019
Xanewok added a commit to rust-lang/rls that referenced this pull request Apr 20, 2019
djrenren pushed a commit to djrenren/compiletest that referenced this pull request Aug 26, 2019
Cosmetic improvements to doc comments

This has been factored out from rust-lang/rust#58036 to only include changes to documentation comments (throughout the rustc codebase).

r? @steveklabnik

Once you're happy with this, maybe we could get it through with r=1, so it doesn't constantly get invalidated? (I'm not sure this will be an issue, but just in case...) Anyway, thanks for your advice so far!
eddyb pushed a commit to LykenSol/rustc_apfloat-git-history-extraction that referenced this pull request Nov 14, 2022
Cosmetic improvements to doc comments

This has been factored out from rust-lang/rust#58036 to only include changes to documentation comments (throughout the rustc codebase).

r? @steveklabnik

Once you're happy with this, maybe we could get it through with r=1, so it doesn't constantly get invalidated? (I'm not sure this will be an issue, but just in case...) Anyway, thanks for your advice so far!


[git filter-repo] original commit: rust-lang/rust@b244f61
spikespaz pushed a commit to spikespaz/dotwalk-rs that referenced this pull request Aug 29, 2024
Cosmetic improvements to doc comments

This has been factored out from rust-lang/rust#58036 to only include changes to documentation comments (throughout the rustc codebase).

r? @steveklabnik

Once you're happy with this, maybe we could get it through with r=1, so it doesn't constantly get invalidated? (I'm not sure this will be an issue, but just in case...) Anyway, thanks for your advice so far!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work.
Projects
None yet
Development

Successfully merging this pull request may close these issues.