Cosmetic improvements to comments #58524

alexreg · 2019-02-16T22:22:49Z

This has been factored out from #58036.

rust-highfive · 2019-02-16T22:22:54Z

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

alexreg · 2019-02-16T23:43:19Z

@matthewjasper I see you got auto-chosen, but please don’t feel obliged to review. (Or maybe just review part, if you're not too averse to it.)

petrochenkov · 2019-02-17T06:41:04Z

src/librustc_privacy/lib.rs

@@ -986,7 +986,7 @@ impl<'a, 'tcx> Visitor<'tcx> for TypePrivacyVisitor<'a, 'tcx> {
            }
        } else {
            // Types in signatures.
-            // FIXME: This is very ineffective. Ideally each HIR type should be converted
+            // FIXME: this is very ineffective. Ideally, each HIR type should be converted


Controversial, please don't change this.

(Case after FIXME.)

petrochenkov · 2019-02-17T06:45:25Z

src/librustc_privacy/lib.rs

@@ -369,7 +369,7 @@ impl VisibilityLike for Option<AccessLevel> {
    const MAX: Self = Some(AccessLevel::Public);
    // Type inference is very smart sometimes.
    // It can make an impl reachable even some components of its type or trait are unreachable.
-    // E.g. methods of `impl ReachableTrait<UnreachableTy> for ReachableTy<UnreachableTy> { ... }`
+    // E.g., methods of `impl ReachableTrait<UnreachableTy> for ReachableTy<UnreachableTy> { ... }`


E.g.,

Another uglification that we shouldn't do IMO.

petrochenkov · 2019-02-17T06:47:14Z

src/librustc_resolve/build_reduced_graph.rs

        // but metadata cannot encode gensyms currently, so we create it here.
        // This is only a guess, two equivalent idents may incorrectly get different gensyms here.
        let ident = ident.gensym_if_underscore();
        let def_id = def.def_id();
-        let expansion = Mark::root(); // FIXME(jseyfried) intercrate hygiene
+        // FIXME(jseyfried): inter-crate hygiene.


Please don't move trailing comments into separate lines.

Well, trailing comments are discouraged in all the codebases I have contributed to, I'll say that much. It decreases readability and is especially problematic when there's a line length cap, I find. It may be worth a brief debate.

petrochenkov · 2019-02-17T07:15:10Z

Ok, I think I'm personally converged on encouraging @alexreg to stop this activity, which is a waste of everyone's (including alexreg himself) time and disruption for the codebase.
We need to treat PRs like this as we would treat manual formatting PRs.

Everything that can be automated should go into rustfmt, and be performed on each commit/PR when the corresponding infra is up.
Typos are caught by occasional codespell runs.
Drive-by non-automatable changes can be done in code the PR author changes for non-cosmetic reasons.

Personal projects may satisfy the irresistible urge to style code in non-automated ways, if necessary.

EDIT: I'm sufficiently sure that if #58036 and friends are merged, we'll see further similar over 1000 diff PRs, e.g. tweaking variable naming across the codebase to match the author's preferred style, because there are always things to "improve" if you have the right attitude. That's why I think this needs to be nipped in the bud.

EDIT2: Previous comment removed since this wasn't my primary point.

bors · 2019-02-17T10:21:22Z

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

petrochenkov · 2019-02-18T20:18:08Z

Since I already got involved into this, I'll try to move this into more constructive direction and describe what I think would be an acceptable way to make changes like this.

Adding a Style Rule, The Process

Suppose you want Rust codebase to follow a certain stylistic rule, for example a rule that comments describing functions should use third person verbs (// Builds a thing) rather than imperative (// Build a thing). Here are the steps to follow.

Add implementation of the rule to the Style Checker (1).
The implementation should enforce the style rule automatically, but can perhaps be more conservative than a human would be (90%).
For example, // VERB would be replaced with // VERBs only for some subset of verbs that can be commonly encountered in comments describing functions (like "builds", "ensures", etc).
Open a PR adding description of the rule to the Rustc Guide Page (2).
Open a PR applying that single rule to the codebase.
ruts-lang/compiler is pinged and the PR enters the waiting-on-team state for ~7 days (something similar to FCPs). ruts-lang/compiler members are free to ignore the PR, leave an approving comment, or leave a disapproving comment.
PRs are merged if they have consensus after the "FCP" period (the new style rule is adopted), or not merged otherwise (the new style rule is rejected).
Once the rule is approved and merged, the remaining 10% may be applied manually, but improving the Style Checker is preferable.

Components

Style Checker.
A tool (Python script, tidy rule, rustfmt rule, whatever) that can be run to enforce the Style Rules automatically. It's not necessarily run on CI for a start, and can be run once a week/month/quarter by any interested person, similarly to codespell. It could be integrated into CI in the future.
Rustc Guide Page.
A page in https://github.com/rust-lang/rustc-guide cataloging the adopted Style Rules.

petrochenkov · 2019-02-18T20:24:46Z

This PR is not meeting any of the requirements so far - the rules are 1) not described anywhere, 2) not applicable automatically, 3) lumped into a single batch and 4) have controversies.

alexreg · 2019-02-18T21:10:18Z

@petrochenkov Thanks for trying to be more constructive with your feedback regarding this all. I think going forwards that makes a lot of sense. However, because of the way I worked on this PR, the changes are all mixed together a bit much and very hard to separate either mechanically or by hand. I think that unless there are changes in this PR that are particularly controversial, and exhibit styles that don't already exist in the codebase, they should be permitted for the sake of getting the uncontroversial and universally beneficial aspects of this PR merged. Would appreciate thoughts on this however.

petrochenkov · 2019-02-19T06:20:34Z

@alexreg
I'm quite strongly against landing this as is, because that would mean encouraging waste of time and bad practices.
Non-controversial parts of the PR are not bug fixes or features, they are low-benefit stylistic changes and cannot counterbalance that.

because of the way I worked on this PR, the changes are all mixed together a bit much and very hard to separate either mechanically or by hand

This is unfortunate, but sunk cost is not a good base for making decisions.
The way this was worked on is exactly the reason why I don't want this merged.
This problem would not exist if the changes were automated, then you could simply rerun the checker and submit PRs proposing individual style rules.

I think that unless there are changes in this PR that are particularly controversial

The changes I mentioned in the inline comments are controversial.
We could discuss details in individual PRs proposing style rules for them.

petrochenkov · 2019-02-19T07:49:20Z

I'm going to nominate this to discuss the proposed process (#58524 (comment)) on Thursday and hopefully close this question.

petrochenkov · 2019-02-23T21:19:16Z

Blocked on #58619

petrochenkov · 2019-03-08T10:29:54Z

Closing as decided in #58619

rust-highfive assigned matthewjasper Feb 16, 2019

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

This comment has been minimized.

Sign in to view

Alexander Regueiro added 4 commits February 16, 2019 22:41

rustc: comments

d963910

tests: comments

cd7ca5e

libs: comments

bc52e15

tools: comments

cf4bc34

alexreg force-pushed the cosmetic-2-comments branch from 28900f6 to cf4bc34 Compare February 16, 2019 22:42

petrochenkov reviewed Feb 17, 2019

View reviewed changes

oli-obk mentioned this pull request Feb 21, 2019

Cosmetic changes to compiler comments and docs #58619

Closed

petrochenkov closed this Mar 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cosmetic improvements to comments #58524

Cosmetic improvements to comments #58524

alexreg commented Feb 16, 2019

rust-highfive commented Feb 16, 2019

This comment has been minimized.

alexreg commented Feb 16, 2019 •

edited

Loading

petrochenkov Feb 17, 2019

petrochenkov Feb 17, 2019

petrochenkov Feb 17, 2019

petrochenkov Feb 17, 2019

alexreg Feb 17, 2019

petrochenkov commented Feb 17, 2019 •

edited

Loading

bors commented Feb 17, 2019

petrochenkov commented Feb 18, 2019 •

edited

Loading

petrochenkov commented Feb 18, 2019

alexreg commented Feb 18, 2019

petrochenkov commented Feb 19, 2019

petrochenkov commented Feb 19, 2019

petrochenkov commented Feb 23, 2019

petrochenkov commented Mar 8, 2019

Cosmetic improvements to comments #58524

Cosmetic improvements to comments #58524

Conversation

alexreg commented Feb 16, 2019

rust-highfive commented Feb 16, 2019

This comment has been minimized.

alexreg commented Feb 16, 2019 • edited Loading

petrochenkov Feb 17, 2019

Choose a reason for hiding this comment

petrochenkov Feb 17, 2019

Choose a reason for hiding this comment

petrochenkov Feb 17, 2019

Choose a reason for hiding this comment

petrochenkov Feb 17, 2019

Choose a reason for hiding this comment

alexreg Feb 17, 2019

Choose a reason for hiding this comment

petrochenkov commented Feb 17, 2019 • edited Loading

bors commented Feb 17, 2019

petrochenkov commented Feb 18, 2019 • edited Loading

Adding a Style Rule, The Process

Components

petrochenkov commented Feb 18, 2019

alexreg commented Feb 18, 2019

petrochenkov commented Feb 19, 2019

petrochenkov commented Feb 19, 2019

petrochenkov commented Feb 23, 2019

petrochenkov commented Mar 8, 2019

alexreg commented Feb 16, 2019 •

edited

Loading

petrochenkov commented Feb 17, 2019 •

edited

Loading

petrochenkov commented Feb 18, 2019 •

edited

Loading