A few cleanups and minor improvements to typeck #54591

ljedrz · 2018-09-26T15:38:33Z

This PR complements #54533, which was limited to check.

change a few push loops to extends
prefer to_owned to to_string for string literals
prefer if let to match where only one branch matters
a few other minor improvements
whitespace fixes

rust-highfive · 2018-09-26T15:38:38Z

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

zackmdavis

Whitespace-only changes could have been a separate commit; bulleted list in PR description could have gone in commit messages (code archeology often benefits from having information in the repository itself, as well as on GitHub).

zackmdavis · 2018-09-27T03:17:42Z

src/librustc_typeck/check_unused.rs

@@ -75,7 +75,7 @@ impl<'a, 'tcx> CheckVisitor<'a, 'tcx> {
        let msg = if let Ok(snippet) = self.tcx.sess.source_map().span_to_snippet(span) {
            format!("unused import: `{}`", snippet)
        } else {
-            "unused import".to_string()
+            "unused import".to_owned()


I also prefer .to_owned over .to_string for literals (the former indicates that we're getting ownership of something which is already a string in some sense of the word, whereas the latter could be anything Display-able), but it's possible that we're in the minority insofar as The Book recommends .to_string or String::from.

I think The Book just recommends the solution that is simplest to grasp and remember; to_string calls String::from which, in turn, calls to_owned(), so everything boils down to to_owned anyways.

zackmdavis · 2018-09-27T03:30:59Z

src/librustc_typeck/astconv.rs

-                            "missing associated type `{}` value", assoc_item.ident))
-                        .emit();
+            struct_span_err!(tcx.sess, span, E0191, "the value of the associated type `{}` \
+                                                     (from the trait `{}`) must be specified",


Someday we should probably get rustfmt working on this codebase (which might require some special tooling; compare #53896). I unambiguously agree with most of the spacing changes in this PR, but where my personal æsthetics happen to disagree (here, I would treat the format varargs the same as the other arguments to struct_span_err! rather than aligning them to the format string), I tend to doubt that that degree of bikeshedding is part of my duty as a reviewer.

Yeah, I guess at one point whitespace tidying will become automated; just I adjust some spots as I go to what I feel looks more readable and hope I'm not the one who feels that way 😜.

zackmdavis · 2018-09-27T03:39:51Z

@ljedrz thanks!

@bors r+

bors · 2018-09-27T03:39:52Z

📌 Commit c8c3395 has been approved by zackmdavis

…davis A few cleanups and minor improvements to typeck This PR complements rust-lang#54533, which was limited to `check`. - change a few `push` loops to `extend`s - prefer `to_owned` to `to_string` for string literals - prefer `if let` to `match` where only one branch matters - a few other minor improvements - whitespace fixes

ljedrz · 2018-09-27T07:26:50Z

@zackmdavis thanks for the review! I'll try to be a bit more specific with my commits in the future.

bors · 2018-09-28T19:12:27Z

⌛ Testing commit c8c3395 with merge 321da78633faaf2b6f2f88277deba5b53c7f766d...

bors · 2018-09-28T19:38:16Z

💔 Test failed - status-travis

rust-highfive · 2018-09-28T19:38:18Z

The job dist-aarch64-linux 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.

151412 ./src/tools/clang
149124 ./src/llvm-emscripten/test
148656 ./obj/build/bootstrap/debug/incremental
134216 ./obj/build/bootstrap/debug/incremental/bootstrap-2zc4gzhr0d54q
134212 ./obj/build/bootstrap/debug/incremental/bootstrap-2zc4gzhr0d54q/s-f58dzdre87-19xvc4g-abll1u66rzrn
104700 ./src/tools/lldb
98940 ./obj/build/x86_64-unknown-linux-gnu/stage0/lib/rustlib/x86_64-unknown-linux-gnu/codegen-backends
93748 ./src/tools/clang/test
72532 ./src/llvm/lib
---
travis_time:end:0b686af4:start=1538163417717083938,finish=1538163417721589856,duration=4505918
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:116a135c
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:160b8cb4
travis_time:start:160b8cb4
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:3d4eac7c
$ dmesg | grep -i kill

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)

ljedrz · 2018-09-28T19:55:06Z

The error seems spurious.

alexcrichton · 2018-09-28T20:29:41Z

@bors: retry

bors · 2018-09-29T06:24:36Z

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

ljedrz · 2018-09-29T09:14:06Z

Rebased.

zackmdavis · 2018-09-29T18:43:03Z

@bors r+

bors · 2018-09-29T18:43:04Z

📌 Commit 608c395 has been approved by zackmdavis

bors · 2018-09-29T22:20:10Z

⌛ Testing commit 608c395 with merge e0a1ab6...

A few cleanups and minor improvements to typeck This PR complements #54533, which was limited to `check`. - change a few `push` loops to `extend`s - prefer `to_owned` to `to_string` for string literals - prefer `if let` to `match` where only one branch matters - a few other minor improvements - whitespace fixes

bors · 2018-09-30T00:51:12Z

☀️ Test successful - status-appveyor, status-travis
Approved by: zackmdavis
Pushing e0a1ab6 to master...

rust-highfive assigned zackmdavis Sep 26, 2018

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

zackmdavis approved these changes Sep 27, 2018

View reviewed changes

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 Sep 27, 2018

kennytm mentioned this pull request Sep 27, 2018

Rollup of 6 pull requests #54607

Closed

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 28, 2018

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 Sep 28, 2018

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 29, 2018

A few cleanups and minor improvements to typeck

608c395

ljedrz force-pushed the cleanup_typeck_rest branch from c8c3395 to 608c395 Compare September 29, 2018 09:13

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 29, 2018

bors merged commit 608c395 into rust-lang:master Sep 30, 2018

ljedrz deleted the cleanup_typeck_rest branch September 30, 2018 05:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

A few cleanups and minor improvements to typeck #54591

A few cleanups and minor improvements to typeck #54591

ljedrz commented Sep 26, 2018

rust-highfive commented Sep 26, 2018

zackmdavis left a comment

zackmdavis Sep 27, 2018

ljedrz Sep 27, 2018

zackmdavis Sep 27, 2018

ljedrz Sep 27, 2018

zackmdavis commented Sep 27, 2018

bors commented Sep 27, 2018

ljedrz commented Sep 27, 2018

bors commented Sep 28, 2018

bors commented Sep 28, 2018

rust-highfive commented Sep 28, 2018

ljedrz commented Sep 28, 2018

alexcrichton commented Sep 28, 2018

bors commented Sep 29, 2018

ljedrz commented Sep 29, 2018

zackmdavis commented Sep 29, 2018

bors commented Sep 29, 2018

bors commented Sep 29, 2018

bors commented Sep 30, 2018

A few cleanups and minor improvements to typeck #54591

A few cleanups and minor improvements to typeck #54591

Conversation

ljedrz commented Sep 26, 2018

rust-highfive commented Sep 26, 2018

zackmdavis left a comment

Choose a reason for hiding this comment

zackmdavis Sep 27, 2018

Choose a reason for hiding this comment

ljedrz Sep 27, 2018

Choose a reason for hiding this comment

zackmdavis Sep 27, 2018

Choose a reason for hiding this comment

ljedrz Sep 27, 2018

Choose a reason for hiding this comment

zackmdavis commented Sep 27, 2018

bors commented Sep 27, 2018

ljedrz commented Sep 27, 2018

bors commented Sep 28, 2018

bors commented Sep 28, 2018

rust-highfive commented Sep 28, 2018

ljedrz commented Sep 28, 2018

alexcrichton commented Sep 28, 2018

bors commented Sep 29, 2018

ljedrz commented Sep 29, 2018

zackmdavis commented Sep 29, 2018

bors commented Sep 29, 2018

bors commented Sep 29, 2018

bors commented Sep 30, 2018