Various cosmetic and minor changes #57016

alexreg · 2018-12-20T18:59:02Z

r? @Centril :-D

Centril · 2018-12-20T19:21:57Z

For the purposes of review I'd prefer if you could:

separate code changes from changes to comments into different PRs.
separate various changes with done with regexes from each other into different PRs.

That's both easier to review and much easier to get through bors.

alexreg · 2018-12-20T19:29:12Z

@Centril sure, will do later.

scalexm · 2018-12-20T19:54:38Z

Before provoking a lot of merge conflicts again, would it be possible for some other people of e.g., @rust-lang/docs @rust-lang/compiler to confirm that these kinds of PRs are wanted on a regular basis?

nikic · 2018-12-20T19:58:54Z

src/libcore/char/methods.rs

@@ -669,7 +669,7 @@ impl char {
        }
    }

-    /// Returns true if this `char` is alphanumeric, and false otherwise.
+    /// Returns whether this `char` is alphanumeric, and false otherwise.


The "and false otherwise" part no longer makes sense if this construction is used. (Same applies to many other places.)

I prefer true if but not being a native english speaker, not sure which is "better"...

“Whether” is definitely better, as a native speaker. They’re both grammatically correct, but “whether” is more succinct and to the point. It’s like writing boolval as opposed to if boolval { true } else { false }, which is an anti-pattern by anyone’s measure.

@nikic Good point, I'll remedy that. It's the problem with find-and-replace.

I personally think true if is a bit better/easier. I don't find it more succinct; it's literally the same number of characters. Additionally, true if is something that's very direct for programmers, even if whether might make more sense outside of a programming context.

Given my experience with English teachers (I was accepted to grad school in English), in my experience they're much less dogmatic than that.

What you call dogma I call respect for other people's time. Let's not fill this thread with anecdotes about our various formal-English-writing teachers, ye? I've had them too and they would care about this.

My view is that, we should aspire to the same standards academic papers have in at least the reference and imo also the standard library. The rustc code-base doesn't need the same level of rigor. We should however also not try to be intentionally casual when someone does the work, as @alexreg has, of improving writing style.

I fundamentally do not believe this to be an improvement.

(The other changes? Very much so. This one? Not so much.)

@steveklabnik Reasonable minds can disagree. :)

“Whether” is definitely better, as a native speaker

I probably prefer true if exactly because I'm not a native speaker.
I probably can't even pronounce "whether" properly, and it took me years to discern it from "weather".
It's better for technical documentation to use some kind of simplified, a bit pseudocode-like version of English, to be clear for larger audience. There's a long history behind this general idea.

@petrochenkov If you had to pronounce the word when reading documentation, you'd possibly have a good point... (it's pronounced exactly the same in most English dialects, but is almost always easy to distinguish from context). In any case, "whether" is definitely part of elementary English vocabulary, so I'm not sure how strong an argument it is for written English.

@steveklabnik While do I strongly feel this is an improvement (it's not a matter of dogma so much as expressivity or directness, to me), I am however happy to let this stand as a subjective point, and defer to your preference, if it helps get this PR merged. :-) I know the PR will be discussed by several people in the next lang team meeting though, so it presumably won't be a decision taken by one person, at least not up-front...

nikomatsakis · 2018-12-20T21:20:24Z

Regarding whether these sorts of changes are desirable, i'm of two minds. They are annoying when it comes to merge conflicts -- but on the other hand it is generally nice to have a codebase with typos fixed and things "cleaned up and consistent" (that said, I think some of these changes -- e.g., changing to "returns whether" from "returns true if" -- are not clearly better to me, just different).

I guess in general my attitude has been "hey, if somebody wants to make the changes and they seem neutral-to-good, why not".

Curious though to hear what others think.

One thing I definitely do prefer is to see the changes factored out into distinct commits (or, in this case, PRs).

petrochenkov · 2018-12-20T21:39:44Z

-0.5 from me.
I'm ok with formatting changes like "two spaces" -> "one space" (they would better be done by rustfmt though) and with "variable_name" -> "variable_name".
But other changes either make things worse ("true if" -> "whether"), or longer ("vs" -> "versus", moving comments to separate lines), or weirder ("def-id" -> "def-ID"), or just randomly change things in accordance with some subjective rules that I can't infer.
Also, churn is bad in general, I want git blame to lead to people who actually wrote the code rather than formatting changes.

alexreg · 2018-12-20T22:30:59Z

@scalexm You’re really trying to make life difficult for me, aren’t you haha? 😄 I’m only trying to improve the messy aspects of the codebase.... Anyway I’ll consult team members on this.

scalexm · 2018-12-20T22:40:25Z

@alexreg No that isn’t my goal at all! It’s just that I am unsure this is wanted by everyone, so as part of this community, I’d like at least a few voices to support that kind of changes before moving forward. Of course if there is a general consensus I’ll have no further objections.

alexreg · 2018-12-20T22:47:36Z

@scalexm Fair enough. I was only half serious anyway. It's certainly worth getting the opinion of the compiler team, whose opinions matter most here, followed by frequent contributors to the codebase like you or me. I know you're against it mainly, but let's keep weighing things up...

alexreg · 2018-12-20T22:57:51Z

@petrochenkov

But other changes either make things worse ("true if" -> "whether"), or longer ("vs" -> "versus", moving comments to separate lines), or weirder ("def-id" -> "def-ID"), or just randomly change things in accordance with some subjective rules that I can't infer.

"Whether" is definitely better, for the reason I stated above. I think you'll either have to trust a native speaker on this (though I'm sure you could find some who dispute my view)... or we'll just agree to disagree. :-)

I don't think "ID" is weirder. "ID" is always capitalised in English, so why not here too, as part of a hyphenated word? The problem is, all rules of natural language are subjective to varying degrees. There's no black and white. My discussion started on the internals forum isn't going to make progress any time soon, if it does, so this is the next best thing in my view.

Also, churn is bad in general, I want git blame to lead to people who actually wrote the code rather than formatting changes.

I think the downside is greatly exaggerated. One can easily look beyond the last edit. The idea is that going forwards, people will hopefully start conforming to a style consensus, even before it gets formalised (if it ever does). I do realise it's marginally harder to look at the blame history of a line or file beyond a certain commit, but it's only marginally.

@nikomatsakis I basically agree with that. On the plus side, these sorts of large cosmetic PRs will be few and far-between, and the conflicts are a bit annoying, but not that bad (usually very easy and quick to fix in my experience). I guess it's half me feeling in some sort of "obsessive-compulsive" frame of mind that makes me do these things, but I also genuinely feel they contribute to the long-term health of the codebase. :-)

bors · 2018-12-21T00:53:25Z

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

rust-highfive · 2018-12-21T06:41:25Z

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

travis_time:end:0161cece:start=1545370749328916942,finish=1545370752255872052,duration=2926955110
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-6.0
---
[01:01:04] ..........................................ii........................................................ 3600/5192
[01:01:05] ............................................................i....................................... 3700/5192
[01:01:07] .................................................................................................... 3800/5192
[01:01:08] .................i.................................................................................. 3900/5192
[01:01:11] ........................F........................................................................... 4000/5192
[01:01:24] .................................................................................................... 4200/5192
[01:01:27] .................................................................................................... 4300/5192
[01:01:31] .......................................................i............................................ 4400/5192
[01:01:37] .................................................................................................... 4500/5192

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

Zoxc · 2018-12-21T08:30:06Z

I generally want to avoid churn unless there is a clear non-subjective improvement.

For the changes in this PR, there are several things I disagree with:

The removal of github links. I really want to keep those as it's very clear what they refer to and they are easy to open with a click.
The use of backticks in comments. I think that just adds unnecessary noise.
Not capitalizing sentences in FIXME comments.

bors · 2018-12-21T13:42:22Z

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

alexreg · 2018-12-21T15:52:07Z

@Zoxc Let me offer some rationale.

The removal of github links. I really want to keep those as it's very clear what they refer to and they are easy to open with a click.

a) There was inconsistent use of "issue #xyz" vs. URLs, b) URLs are noisy in comments and make them harder to read

The use of backticks in comments. I think that just adds unnecessary noise.

It makes it very clear what is not part of the normal English sentence (not just code, but metavariables and other expressions), and what isn't. Seems you're in a minority here, but not sure.

Not capitalizing sentences in FIXME comments.

This is minor, of course. It's a standard grammatical rule not to follow a colon with capitalisation though.

nikic · 2018-12-21T16:13:26Z

This is minor, of course. It's a standard grammatical rule not to follow a colon with capitalisation though.

Style guides sometimes (e.g. AP) distinguish whether the words after the colon are a dependent clause, and use capitalization if it isn't. In the case of TODO/FIXME, what follows will usually not be a dependent clause and thus should be capitalized. Sometimes a distinction is made based on whether the colon is followed by one or by more than one sentences (e.g. Chicago).

I don't particularly care either way, but I think it's important to acknowledge that these changes are ultimately based on your personal preference in the choice of a particular style guide, not on normative rules of the English language. And that makes it hard to agree on things, because people will have different personal preferences.

alexreg · 2018-12-21T16:19:59Z

I don't particularly care either way, but I think it's important to acknowledge that these changes are ultimately based on your personal preference in the choice of a particular style guide, not on normative rules of the English language. And that makes it hard to agree on things, because people will have different personal preferences.

This may be an American English thing. In fact, I just looked it up, and apparently we Brits always use lower case after colons, but Americans often follow the rule you stated above.

Centril · 2018-12-21T16:55:40Z

I mostly agree with @nikomatsakis.

On a tangent, I think that a more serious problem and a barrier to entry is the huge size of some files and functions/methods in the rust repo. Having >= 7000 LOC files strikes me as unmaintainable / unreadable and might point to larger architectural problem around APIs that are hard to refactor (possibly due to having too much context / being too stateful). A more healthy size would be ~300-1000 LOC per file imo.

bors · 2018-12-22T23:27:15Z

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

bors · 2018-12-26T18:55:04Z

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

nikomatsakis · 2018-12-28T16:13:36Z

In general, I would be careful @alexreg in declaring anything to be "objectively" better, particularly when it comes to English, which is certainly an unruly language lacking any kind of "centralized" authority (I can't say if the same is true for other languages). =)

Definitely things like whether to capitalize after a colon have a variety of rules (amusingly, this exact question came up recently for me as I was trying to edit some of my daughter's homework, and couldn't predict what rules the teacher might be using).

My personal take is that I prefer backticks in comments to identify variable names and the like and don't really care about the rest. e.g., a URL is fine, but it's also ok to write #2123, as long as I can easily find the thing in question.

As for the fate of this PR, I'm not yet sure what to do exactly. As I wrote before, I don't object to merging it. I do however have reservations about trying to standardize rules on this sort of thing -- I'm not eager to be nitpicking PRs on random grammatical rules like whether to capitalize a sentence fragment after the word "whether". (Note: Here I am referring specifically to internal comments -- public-facing documentation is another thing.)

One thing I did notice is that this PR edits both internal rustc APIs as well as the documentation for things in libstd. It does strike me as reasonable to have a "style guide" for the latter, but I would expect @rust-lang/docs to be involved in developing it. It might be wise to separate out those changes, ultimately, not sure.

For the time being, I've nominated the PR for discussion at a @rust-lang/compiler meeting.

steveklabnik · 2018-12-28T16:15:47Z

The docs team has two merged RFCs related to style for public comments, incidentally.

…

On Dec 28, 2018, at 11:13 AM, Niko Matsakis ***@***.***> wrote: In general, I would be careful @alexreg in declaring anything to be "objectively" better, particularly when it comes to English, which is certainly an unruly language lacking any kind of "centralized" authority (I can't say if the same is true for other languages). =) Definitely things like whether to capitalize after a colon have a variety of rules (amusingly, this exact question came up recently for me as I was trying to edit some of my daughter's homework, and couldn't predict what rules the teacher might be using). My personal take is that I prefer backticks in comments to identify variable names and the like and don't really care about the rest. e.g., a URL is fine, but it's also ok to write #2123, as long as I can easily find the thing in question. As for the fate of this PR, I'm not yet sure what to do exactly. As I wrote before, I don't object to merging it. I do however have reservations about trying to standardize rules on this sort of thing -- I'm not eager to be nitpicking PRs on random grammatical rules like whether to capitalize a sentence fragment after the word "whether". (Note: Here I am referring specifically to internal comments -- public-facing documentation is another thing.) One thing I did notice is that this PR edits both internal rustc APIs as well as the documentation for things in libstd. It does strike me as reasonable to have a "style guide" for the latter, but I would expect @rust-lang/docs to be involved in developing it. It might be wise to separate out those changes, ultimately, not sure. For the time being, I've nominated the PR for discussion at a @rust-lang/compiler meeting. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

nikomatsakis · 2018-12-28T16:18:10Z

The docs team has two merged RFCs related to style for public comments, incidentally.

Yes, but I don't recall whether they addressed these specific questions.

nikomatsakis · 2018-12-28T16:09:31Z

src/liballoc/string.rs

@@ -1388,7 +1388,7 @@ impl String {
        self.vec.len()
    }

-    /// Returns `true` if this `String` has a length of zero.
+    /// Returns whether this `String` has a length of zero.


Example of a change affecting public-facing documentation. That feels to me like a @rust-lang/docs decision.

Fair point. The quality of public-facing documentation in rustc currently isn't great (at least compared to the stdlib and some libraries), but it would be nice if it reaches that level some day, and if the style is consistent with the stdlib.

alexreg · 2018-12-28T16:46:54Z

Thanks for the nomination @nikomatsakis. English does lack a central authority, as you say (Spanish and French notably have one, I believe). That said, there are certain things that are for all intents and purposes objective, and others that are more subjective or dialectal – they all fall on a spectrum though.

bors · 2018-12-29T23:46:51Z

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

bors · 2019-01-01T02:09:39Z

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

bors · 2019-01-01T18:03:22Z

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

pnkfelix · 2019-01-17T15:40:28Z

Discussed at T-compiler meeting. Team essentially decided that we do not have any existing style manual dictating what kind rules we follow for our comments, and we are nervous about adopting any such style manual for e.g. what constitutes proper English.

There was general consensus that with the PR as given, its too difficult for us to immediately see distinct categories of the changes here, and therefore it would represent some non-trivial amount of work to build up such a categorization. Which means we cannot effectively have a conversation about the set of changes here, at least not in an exhaustive manner. (So not only can we not accept the PR as is, but we really can't provide great feedback on how to refine it, apart from: Can you break this up into separate commits or even PR's that are themselves separated by category. Which then would represent a lot of potential busy work for you with little payoff.)

We also recognized that discussions on formatting and comment styles are prone to bikeshed and conflict: a potential big drain on developer time.

Our basic consensus on how to move forward is this:

We generally accept drive-by style changes, in the sense that if you happen to be working on a purposeful change to code near by, then people will accept obvious improvements to comment text near there. (But if anyone objects to a change in style, then we will tend to error on the side of keeping the text as is.)
Overall, we just try not to make a big deal about any of this, and do not want to adopt any true "official" rules.

pnkfelix · 2019-01-17T15:41:08Z

closing PR under reasoning given in my previous comment

alexreg · 2019-01-17T17:49:41Z

@pnkfelix So to be clear, if I separate this into a bunch of commits, categorised, would you or another team member take another look at it? I think some of these changes dramatically improve the codebase readability still.

estebank · 2019-01-17T19:40:14Z

@alexreg it'd be even better if they could be done as separate PRs to avoid one type of changes from blocking the others from being merged expeditiously.

alexreg · 2019-01-17T22:05:33Z

@estebank Yeah, but there might be a lot of conflicts then, so I'm very much not a fan of it. It's also just a PITA for me.

oli-obk · 2019-01-18T07:35:51Z

Yeah, but there might be a lot of conflicts then, so I'm very much not a fan of it. It's also just a PITA for me.

I'm not sure if helpful (since you did not mention which parts you find painful), but the way I do PRs like that is by only having a single PR open. Once split into multiple commits, you can open a PR for the first commit, once that is merged, open a PR for the second commit, ... When rebasing, you rebase the PR-branch and then the branch with all commits on top of that. This way you only use up a single workspace and don't need to worry about cross-conflicts between separate PRs.

alexreg · 2019-01-18T15:47:27Z

@oli-obk Sounds fair.

rust-highfive assigned Centril Dec 20, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 20, 2018

This comment has been minimized.

Sign in to view

nikic reviewed Dec 20, 2018

View reviewed changes

alexreg force-pushed the cosmetic-2 branch from 52c8286 to 781bb9d Compare December 21, 2018 05:01

This comment has been minimized.

Sign in to view

alexreg force-pushed the cosmetic-2 branch from 781bb9d to e3802d6 Compare December 21, 2018 05:37

alexreg force-pushed the cosmetic-2 branch from e3802d6 to cf2c1e0 Compare December 21, 2018 15:59

This comment has been minimized.

Sign in to view

alexreg force-pushed the cosmetic-2 branch from cf2c1e0 to 989310b Compare December 23, 2018 04:00

This comment has been minimized.

Sign in to view

alexreg force-pushed the cosmetic-2 branch from 989310b to 7535bcb Compare December 23, 2018 22:34

alexreg force-pushed the cosmetic-2 branch from f23a15b to 0e26a62 Compare December 25, 2018 23:11

Centril assigned nikomatsakis and unassigned Centril Dec 28, 2018

nikomatsakis added I-nominated T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 28, 2018

nikomatsakis reviewed Dec 28, 2018

View reviewed changes

alexreg force-pushed the cosmetic-2 branch from 0e26a62 to 5f3868c Compare December 29, 2018 05:40

This comment has been minimized.

Sign in to view

alexreg force-pushed the cosmetic-2 branch from 5f3868c to 8de50e7 Compare December 29, 2018 16:44

alexreg force-pushed the cosmetic-2 branch from 8de50e7 to 2e9d1c2 Compare December 31, 2018 23:07

Various cosmetic improvements

6434aaf

alexreg force-pushed the cosmetic-2 branch from 2e9d1c2 to 6434aaf Compare January 1, 2019 02:21

pnkfelix closed this Jan 17, 2019

petrochenkov mentioned this pull request Feb 26, 2019

Cosmetic changes to compiler comments and docs #58619

Closed

RalfJung mentioned this pull request May 10, 2024

chore: Remove repeated words (extension of #124924) #124948

Merged

Various cosmetic and minor changes #57016

Various cosmetic and minor changes #57016

Conversation

alexreg commented Dec 20, 2018

Centril commented Dec 20, 2018

This comment has been minimized.

alexreg commented Dec 20, 2018

scalexm commented Dec 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steveklabnik Dec 30, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexreg Jan 1, 2019 • edited

Choose a reason for hiding this comment

nikomatsakis commented Dec 20, 2018

petrochenkov commented Dec 20, 2018

alexreg commented Dec 20, 2018

scalexm commented Dec 20, 2018 • edited

alexreg commented Dec 20, 2018 • edited

alexreg commented Dec 20, 2018

bors commented Dec 21, 2018

This comment has been minimized.

rust-highfive commented Dec 21, 2018

Zoxc commented Dec 21, 2018

bors commented Dec 21, 2018

alexreg commented Dec 21, 2018

nikic commented Dec 21, 2018

alexreg commented Dec 21, 2018

Centril commented Dec 21, 2018

This comment has been minimized.

bors commented Dec 22, 2018

This comment has been minimized.

bors commented Dec 26, 2018

nikomatsakis commented Dec 28, 2018

steveklabnik commented Dec 28, 2018 via email

nikomatsakis commented Dec 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexreg commented Dec 28, 2018

This comment has been minimized.

bors commented Dec 29, 2018

bors commented Jan 1, 2019

bors commented Jan 1, 2019

pnkfelix commented Jan 17, 2019

pnkfelix commented Jan 17, 2019

alexreg commented Jan 17, 2019

estebank commented Jan 17, 2019

alexreg commented Jan 17, 2019

oli-obk commented Jan 18, 2019

alexreg commented Jan 18, 2019

steveklabnik Dec 30, 2018 •

edited

alexreg Jan 1, 2019 •

edited

scalexm commented Dec 20, 2018 •

edited

alexreg commented Dec 20, 2018 •

edited