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

Introduce hir::ExprKind::Use and employ in for loop desugaring. #60225

Merged
merged 1 commit into from Apr 26, 2019

Conversation

Projects
None yet
7 participants
@Centril
Copy link
Member

commented Apr 24, 2019

In the for $pat in $expr $block desugaring we end with a { let _result = $match_expr; _result } construct which makes for loops into a terminating scope and affects drop order. The construct was introduced in year 2015 by @pnkfelix in #21984.

This PR replaces the construct with hir::ExprKind::Use(P<hir::Expr>) which is equivalent semantically but should hopefully be less costly in terms of compile time performance (to be determined).

This is extracted out of 91b0abd from #59288 for easier review and so that the perf implications wrt. for-loops can be measured.

r? @oli-obk

@Centril

This comment has been minimized.

Copy link
Member Author

commented Apr 24, 2019

@bors try

@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2019

⌛️ Trying commit b755b93 with merge 51f54bd...

bors added a commit that referenced this pull request Apr 24, 2019

Auto merge of #60225 - Centril:hir-exprkind-use-in-for-loops, r=<try>
Introduce hir::ExprKind::Use and employ in for loop desugaring.

In the `for $pat in $expr $block` desugaring we end with a `{ let _result = $match_expr; _result }` construct which makes `for` loops into a terminating scope and affects drop order. The construct was introduced in year 2015 by @pnkfelix in #21984.

This PR replaces the construct with `hir::ExprKind::Use(P<hir::Expr>)` which is equivalent semantically but should hopefully be less costly in terms of compile time performance (to be determined).

This is extracted out of 91b0abd from #59288 for easier review and so that the perf implications wrt. `for`-loops can be measured.

r? @oli-obk
Show resolved Hide resolved src/librustc/hir/print.rs Outdated
@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2019

☀️ Try build successful - checks-travis
Build commit: 51f54bd

@Centril

This comment has been minimized.

Copy link
Member Author

commented Apr 24, 2019

@rust-timer

This comment has been minimized.

Copy link

commented Apr 24, 2019

Success: Queued 51f54bd with parent 0928511, comparison URL.

@rust-timer

This comment has been minimized.

Copy link

commented Apr 24, 2019

Finished benchmarking try commit 51f54bd

@Centril Centril force-pushed the Centril:hir-exprkind-use-in-for-loops branch from b755b93 to 487cc54 Apr 25, 2019

@Centril

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2019

@oli-obk Adjusted the pretty printing... :)

@Centril Centril force-pushed the Centril:hir-exprkind-use-in-for-loops branch from 487cc54 to f59f7d4 Apr 25, 2019

Introduce hir::ExprKind::Use and employ in for loop desugaring.
Here, ExprKind::Use(P<Expr>) tweaks the drop order to act the
same way as '{ let _tmp = expr; _tmp }' does.

@Centril Centril force-pushed the Centril:hir-exprkind-use-in-for-loops branch from f59f7d4 to 4bd36ab Apr 25, 2019

@Centril

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2019

@bors r=oli-obk

@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

📌 Commit 4bd36ab has been approved by oli-obk

@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

⌛️ Testing commit 4bd36ab with merge e54563c...

bors added a commit that referenced this pull request Apr 25, 2019

Auto merge of #60225 - Centril:hir-exprkind-use-in-for-loops, r=oli-obk
Introduce hir::ExprKind::Use and employ in for loop desugaring.

In the `for $pat in $expr $block` desugaring we end with a `{ let _result = $match_expr; _result }` construct which makes `for` loops into a terminating scope and affects drop order. The construct was introduced in year 2015 by @pnkfelix in #21984.

This PR replaces the construct with `hir::ExprKind::Use(P<hir::Expr>)` which is equivalent semantically but should hopefully be less costly in terms of compile time performance (to be determined).

This is extracted out of 91b0abd from #59288 for easier review and so that the perf implications wrt. `for`-loops can be measured.

r? @oli-obk
@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

💔 Test failed - checks-travis

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Apr 25, 2019

The job dist-x86_64-apple 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.
voltdb
wry
zxing-cpp
==> Downloading https://homebrew.bintray.com/bottles/xz-5.2.4.high_sierra.bottle.tar.gz
==> Downloading from https://akamai.bintray.com/e7/e7be50f4ee00e35887f3957263334eb3baba59e8c061919060f9259351be6880?__gda__=exp=1556199623~hmac=44cb3c3ae9f6c8daff8db87a0c2f2edc071a1a35673c26185ebeb4acaf099654&response-content-disposition=attachment%3Bfilename%3D%22xz-5.2.4.high_sierra.bottle.tar.gz%22&response-content-type=application%2Fgzip&requestInfo=U2FsdGVkX1-RFQXC3JaiBI9DJ1Gza-DlNGpZ5egEYkRe9wIrxs7RYptHjQFR2mW9dLxrTk2x4NYtv5AE0dDSvzE8ksEz06PgF3QxvwssgSqEU5BNUsRqsj_fAoa5TOvtTjSexCw87FTifza5WoQTzQ&response-X-Checksum-Sha1=32dc0b28e61f32b40c20e2993418aa8cb6e746d5&response-X-Checksum-Sha2=e7be50f4ee00e35887f3957263334eb3baba59e8c061919060f9259351be6880
🍺  /usr/local/Cellar/xz/5.2.4: 92 files, 1MB
==> `brew cleanup` has not been run in 30 days, running now...
Removing: /Users/travis/Library/Caches/Homebrew/boost-1.66.0.high_sierra.bottle.tar.gz... (84.6MB)
Removing: /Users/travis/Library/Caches/Homebrew/carthage-0.28.0.high_sierra.bottle.tar.gz... (8.3MB)
---
Pruned 0 symbolic links and 5 directories from /usr/local
==> Installing dependencies for swig: pcre
==> Installing swig dependency: pcre
==> Downloading https://homebrew.bintray.com/bottles/pcre-8.43.high_sierra.bottle.tar.gz
==> Downloading from https://akamai.bintray.com/03/0389911a93a88efd4a69b52dea8ecb872fdb55bcfff45d2f7313be5f79730861?__gda__=exp=1556199638~hmac=f1d2eaec009e244d5c142096614f83e547b77cacca43a313564683addc301de4&response-content-disposition=attachment%3Bfilename%3D%22pcre-8.43.high_sierra.bottle.tar.gz%22&response-content-type=application%2Fgzip&requestInfo=U2FsdGVkX19C32egHFHAJu_REFzdRokA5h4IPfcQoLqs4grGLobz-Rs5jCpr6MyE_RthseHh6WZu03Jw9ZIBlhR7Wj86rR3c74xiNu3llioKv_yGLosiPoH-Ldv-SmXKPvpC32nXKkfQOgfnVZXhrQ&response-X-Checksum-Sha1=c67d4b99bb245f0ea56b34118dd6325b06a7250c&response-X-Checksum-Sha2=0389911a93a88efd4a69b52dea8ecb872fdb55bcfff45d2f7313be5f79730861
🍺  /usr/local/Cellar/pcre/8.43: 204 files, 5.5MB
==> Installing swig
==> Downloading https://homebrew.bintray.com/bottles/swig-3.0.12.high_sierra.bottle.tar.gz
==> Downloading https://homebrew.bintray.com/bottles/swig-3.0.12.high_sierra.bottle.tar.gz
==> Downloading from https://akamai.bintray.com/c0/c0e2656fd10d57281280d20ce8bf9a060cf8714f4283dd1dfde383b3688d9ed1?__gda__=exp=1556199641~hmac=e7eb55fd718911ebab9614a4925a32d9423faf1831145277c08cb71789b71d49&response-content-disposition=attachment%3Bfilename%3D%22swig-3.0.12.high_sierra.bottle.tar.gz%22&response-content-type=application%2Fgzip&requestInfo=U2FsdGVkX1-dzNqv4DZgDv9Y6sPyujks-LAA-EV1kfaauUj88voI8vMMcVj_yJXTIbZPW9yKAVTu84JjwpUXzlqVC4M0y_NCX8CK59I0-Yx_Wg_v_p7-85sKCahwE01WEfvA2OP9K9R2q_GXwATInQ&response-X-Checksum-Sha1=db6e6ed21965214d5f9fba1b180517bb2587ef59&response-X-Checksum-Sha2=c0e2656fd10d57281280d20ce8bf9a060cf8714f4283dd1dfde383b3688d9ed1
🍺  /usr/local/Cellar/swig/3.0.12: 755 files, 5.5MB
travis_time:end:050a3a4d:start=1556198579370597000,finish=1556198959419646000,duration=380049049000
travis_fold:end:install
travis_fold:start:before_script.1
---
[00:03:07]       Memory: 8 GB
[00:03:07]       Boot ROM Version: VMW71.00V.7581552.B64.1801142334
[00:03:07]       Apple ROM Info: [MS_VM_CERT/SHA1/27d66596a61c48dd3dc7216fd715126e33f59ae7]Welcome to the Virtual Machine
[00:03:07]       SMC Version (system): 2.8f0
[00:03:07]       Serial Number (system): VMuCCsZpProG
[00:03:07] 
[00:03:07] hw.ncpu: 4
[00:03:07] hw.byteorder: 1234
[00:03:07] hw.memsize: 8589934592
---
[00:09:00]    Compiling syntax_pos v0.0.0 (/Users/travis/build/rust-lang/rust/src/libsyntax_pos)
[00:09:05] [RUSTC-TIMING] syntax_pos test:false 5.381
[00:09:05]    Compiling rustc_errors v0.0.0 (/Users/travis/build/rust-lang/rust/src/librustc_errors)
[00:09:14] [RUSTC-TIMING] rustc_errors test:false 8.629
No output has been received in the last 30m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received
The build has been terminated

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)

@oli-obk

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

@bors retry timeout

Centril added a commit to Centril/rust that referenced this pull request Apr 25, 2019

Rollup merge of rust-lang#60225 - Centril:hir-exprkind-use-in-for-loo…
…ps, r=oli-obk

Introduce hir::ExprKind::Use and employ in for loop desugaring.

In the `for $pat in $expr $block` desugaring we end with a `{ let _result = $match_expr; _result }` construct which makes `for` loops into a terminating scope and affects drop order. The construct was introduced in year 2015 by @pnkfelix in rust-lang#21984.

This PR replaces the construct with `hir::ExprKind::Use(P<hir::Expr>)` which is equivalent semantically but should hopefully be less costly in terms of compile time performance (to be determined).

This is extracted out of rust-lang@91b0abd from rust-lang#59288 for easier review and so that the perf implications wrt. `for`-loops can be measured.

r? @oli-obk

Centril added a commit to Centril/rust that referenced this pull request Apr 25, 2019

Rollup merge of rust-lang#60225 - Centril:hir-exprkind-use-in-for-loo…
…ps, r=oli-obk

Introduce hir::ExprKind::Use and employ in for loop desugaring.

In the `for $pat in $expr $block` desugaring we end with a `{ let _result = $match_expr; _result }` construct which makes `for` loops into a terminating scope and affects drop order. The construct was introduced in year 2015 by @pnkfelix in rust-lang#21984.

This PR replaces the construct with `hir::ExprKind::Use(P<hir::Expr>)` which is equivalent semantically but should hopefully be less costly in terms of compile time performance (to be determined).

This is extracted out of rust-lang@91b0abd from rust-lang#59288 for easier review and so that the perf implications wrt. `for`-loops can be measured.

r? @oli-obk

bors added a commit that referenced this pull request Apr 25, 2019

Auto merge of #60291 - Centril:rollup-uat6tsv, r=Centril
Rollup of 10 pull requests

Successful merges:

 - #59734 (Prevent failure in case no space left on device in rustdoc)
 - #60134 (Fix index-page generation)
 - #60165 (Add Pin::{into_inner,into_inner_unchecked})
 - #60183 (Chalkify: Add builtin Copy/Clone)
 - #60225 (Introduce hir::ExprKind::Use and employ in for loop desugaring.)
 - #60247 (Implement Debug for Place using Place::iterate)
 - #60259 (Derive Default instead of new in applicable lint)
 - #60262 ( PGO: Add a run-make test that makes sure that PGO profiling data is used by the compiler during optimizations.)
 - #60267 (Add feature-gate for f16c target feature)
 - #60285 (Do not ICE when checking types against foreign fn)

Failed merges:

r? @ghost

Centril added a commit to Centril/rust that referenced this pull request Apr 26, 2019

Rollup merge of rust-lang#60225 - Centril:hir-exprkind-use-in-for-loo…
…ps, r=oli-obk

Introduce hir::ExprKind::Use and employ in for loop desugaring.

In the `for $pat in $expr $block` desugaring we end with a `{ let _result = $match_expr; _result }` construct which makes `for` loops into a terminating scope and affects drop order. The construct was introduced in year 2015 by @pnkfelix in rust-lang#21984.

This PR replaces the construct with `hir::ExprKind::Use(P<hir::Expr>)` which is equivalent semantically but should hopefully be less costly in terms of compile time performance (to be determined).

This is extracted out of rust-lang@91b0abd from rust-lang#59288 for easier review and so that the perf implications wrt. `for`-loops can be measured.

r? @oli-obk

bors added a commit that referenced this pull request Apr 26, 2019

Auto merge of #60296 - Centril:rollup-qh9la7k, r=Centril
Rollup of 12 pull requests

Successful merges:

 - #59734 (Prevent failure in case no space left on device in rustdoc)
 - #59940 (Set cfg(test) when rustdoc is running with --test option)
 - #60134 (Fix index-page generation)
 - #60165 (Add Pin::{into_inner,into_inner_unchecked})
 - #60183 (Chalkify: Add builtin Copy/Clone)
 - #60225 (Introduce hir::ExprKind::Use and employ in for loop desugaring.)
 - #60247 (Implement Debug for Place using Place::iterate)
 - #60259 (Derive Default instead of new in applicable lint)
 - #60267 (Add feature-gate for f16c target feature)
 - #60284 (Do not allow const generics to depend on type parameters)
 - #60285 (Do not ICE when checking types against foreign fn)
 - #60289 (Make `-Z allow-features` work for stdlib features)

Failed merges:

r? @ghost

@bors bors merged commit 4bd36ab into rust-lang:master Apr 26, 2019

1 of 2 checks passed

homu Test failed
Details
Travis CI - Pull Request Build Passed
Details

@Centril Centril deleted the Centril:hir-exprkind-use-in-for-loops branch Apr 26, 2019

@@ -1486,6 +1488,10 @@ pub enum ExprKind {
Cast(P<Expr>, P<Ty>),
/// A type reference (e.g., `Foo`).
Type(P<Expr>, P<Ty>),
/// Semantically equivalent to `{ let _t = expr; _t }`.
/// Maps directly to `hair::ExprKind::Use`.
/// Only exists to tweak the drop order in HIR.

This comment has been minimized.

Copy link
@Manishearth

Manishearth Apr 27, 2019

Member

Can we have more docs on where this is introduced and why it exists? As it stands it's pretty confusing

This comment has been minimized.

Copy link
@Centril

Centril Apr 27, 2019

Author Member

It is used in the for loop desugaring as seen in this PR and also in #59288 (see lowering.rs).

This comment has been minimized.

Copy link
@Manishearth

Manishearth Apr 27, 2019

Member

I'm not asking you to explain it to me, I figured it out, I'm saying that this should be in the docs.

This comment has been minimized.

Copy link
@Centril

Centril Apr 27, 2019

Author Member

Sure why not. Some parts haven't been merged yet so it would be slightly weird to talk about future events in the docs. Can you file an issue for now`?

This comment has been minimized.

Copy link
@eddyb

eddyb Apr 30, 2019

Member

I don't find the name... useful (sorry). I'd call this SomethingScope, which for the lack of a better option would be TerminatingScope.

I believe that ExprKind::Use elsewhere has nothing to do with scopes (just like mir::Rvalue::Use doesn't), so using Use is misleading.

This comment has been minimized.

Copy link
@Centril

Centril Apr 30, 2019

Author Member

@eddyb Fwiw, I contemplated ExprKind::Terminating. I agree completely that ::Use sucks but I didn't have anything super better at the time.

Manishearth added a commit to rust-lang/rust-clippy that referenced this pull request Apr 27, 2019

Manishearth added a commit to rust-lang/rust-clippy that referenced this pull request Apr 27, 2019

mikerite added a commit to mikerite/rust-clippy that referenced this pull request Apr 28, 2019

bors added a commit to rust-lang/rust-clippy that referenced this pull request Apr 28, 2019

Auto merge of #4040 - mikerite:fix-build-20190428, r=Manishearth
Fix breakage due to rust-lang/rust#60225

Wrote this up before I saw that Manish already started on a fix in #4038. It no doubt contains errors. Feel free to close.

Centril added a commit to Centril/rust that referenced this pull request Apr 30, 2019

Rollup merge of rust-lang#60417 - Centril:hir-exprkind-use-renamed-to…
…-drop-temps, r=oli-obk

Rename hir::ExprKind::Use to ::DropTemps and improve docs.

Addresses rust-lang#60225 (comment).

r? @oli-obk

cc @eddyb @Manishearth

Centril added a commit to Centril/rust that referenced this pull request May 1, 2019

Rollup merge of rust-lang#60417 - Centril:hir-exprkind-use-renamed-to…
…-drop-temps, r=oli-obk

Rename hir::ExprKind::Use to ::DropTemps and improve docs.

Addresses rust-lang#60225 (comment).

r? @oli-obk

cc @eddyb @Manishearth

Centril added a commit to Centril/rust that referenced this pull request May 1, 2019

Rollup merge of rust-lang#60417 - Centril:hir-exprkind-use-renamed-to…
…-drop-temps, r=oli-obk

Rename hir::ExprKind::Use to ::DropTemps and improve docs.

Addresses rust-lang#60225 (comment).

r? @oli-obk

cc @eddyb @Manishearth

Centril added a commit to Centril/rust that referenced this pull request May 1, 2019

Rollup merge of rust-lang#60417 - Centril:hir-exprkind-use-renamed-to…
…-drop-temps, r=oli-obk

Rename hir::ExprKind::Use to ::DropTemps and improve docs.

Addresses rust-lang#60225 (comment).

r? @oli-obk

cc @eddyb @Manishearth
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.