Document From trait for LhsExpr in parser #64136

crgl · 2019-09-03T23:01:13Z

Add doc for From trait for converting P and Option<ThinVec> to LhsExpr

As part of issue #51430 (cc @skade).

Both of these should just be moving an address and setting a discriminant in an enum. The main thing I'm not sure about is whether it's worth documenting the branch in the From<Option<ThinVec>. As far as I can tell it doesn't seem like it is optimized away (although if the discriminant happened to work out you could just copy the pointer and the discriminant which might be cheaper, but that's not guaranteed). So it seems like if it's being called often, it's doubling the number of possible branch mispredictions on this Option, which could be a significant cost.

Let me know if there's anything that needs fixing and I'll get to it as soon as possible!

rust-highfive · 2019-09-03T23:01:16Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @eddyb (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

rust-highfive · 2019-09-03T23:10:12Z

The job mingw-check of your PR failed (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.

2019-09-03T23:01:50.0551941Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-09-03T23:01:50.0737230Z ##[command]git config gc.auto 0
2019-09-03T23:01:50.0822149Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-09-03T23:01:50.0911783Z ##[command]git config --get-all http.proxy
2019-09-03T23:01:50.1057734Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/64136/merge:refs/remotes/pull/64136/merge
---
2019-09-03T23:10:06.9562794Z == clock drift check ==
2019-09-03T23:10:06.9581515Z   local time: Tue Sep  3 23:10:06 UTC 2019
2019-09-03T23:10:07.0303508Z   network time: Tue, 03 Sep 2019 23:10:07 GMT
2019-09-03T23:10:07.0309235Z == end clock drift check ==
2019-09-03T23:10:08.3394067Z ##[error]Bash exited with code '1'.
2019-09-03T23:10:08.3429595Z ##[section]Starting: Checkout
2019-09-03T23:10:08.3431595Z ==============================================================================
2019-09-03T23:10:08.3431673Z Task         : Get sources
2019-09-03T23:10:08.3431727Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

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)

rust-highfive · 2019-09-04T17:45:10Z

The job x86_64-gnu-llvm-6.0 of your PR failed (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.

2019-09-04T17:25:02.7194233Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-09-04T17:25:02.7375931Z ##[command]git config gc.auto 0
2019-09-04T17:25:02.7496166Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-09-04T17:25:02.7546499Z ##[command]git config --get-all http.proxy
2019-09-04T17:25:02.7693604Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/64136/merge:refs/remotes/pull/64136/merge
---
2019-09-04T17:32:12.8335394Z    Compiling serde_json v1.0.40
2019-09-04T17:32:14.6221399Z    Compiling tidy v0.1.0 (/checkout/src/tools/tidy)
2019-09-04T17:32:25.7295601Z     Finished release [optimized] target(s) in 1m 29s
2019-09-04T17:32:25.7374063Z tidy check
2019-09-04T17:32:25.8644644Z tidy error: /checkout/src/libsyntax/parse/parser/expr.rs:70: trailing whitespace
2019-09-04T17:32:25.8645462Z tidy error: /checkout/src/libsyntax/parse/parser/expr.rs:83: trailing whitespace
2019-09-04T17:32:27.6528089Z some tidy checks failed
2019-09-04T17:32:27.6533428Z 
2019-09-04T17:32:27.6533428Z 
2019-09-04T17:32:27.6534875Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor"
2019-09-04T17:32:27.6535030Z 
2019-09-04T17:32:27.6535057Z 
2019-09-04T17:32:27.6545234Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
2019-09-04T17:32:27.6545384Z Build completed unsuccessfully in 0:01:33
2019-09-04T17:32:27.6545384Z Build completed unsuccessfully in 0:01:33
2019-09-04T17:32:27.6598675Z == clock drift check ==
2019-09-04T17:32:27.6612531Z   local time: Wed Sep  4 17:32:27 UTC 2019
2019-09-04T17:32:27.8945621Z   network time: Wed, 04 Sep 2019 17:32:27 GMT
2019-09-04T17:32:27.8949601Z == end clock drift check ==
2019-09-04T17:32:29.4495130Z ##[error]Bash exited with code '1'.
2019-09-04T17:32:29.4537795Z ##[section]Starting: Checkout
2019-09-04T17:32:29.4539545Z ==============================================================================
2019-09-04T17:32:29.4539592Z Task         : Get sources
2019-09-04T17:32:29.4539651Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

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)

JohnCSimon · 2019-09-14T01:45:37Z

Ping from triage
@eddyb @Centril This PR is still waiting on review. Thanks.

CC: @cr1901

src/libsyntax/parse/parser/expr.rs

Centril · 2019-09-14T11:36:10Z

r? @Centril

r=me rollup with suggestions applied.

rust-highfive · 2019-09-15T22:08:57Z

The job mingw-check of your PR failed (pretty log, 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.

2019-09-15T21:57:07.4358346Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-09-15T21:57:07.4576298Z ##[command]git config gc.auto 0
2019-09-15T21:57:07.4652382Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-09-15T21:57:07.4721995Z ##[command]git config --get-all http.proxy
2019-09-15T21:57:08.0727824Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/64136/merge:refs/remotes/pull/64136/merge
---
2019-09-15T22:05:25.5043785Z    Compiling rustc_macros v0.1.0 (/checkout/src/librustc_macros)
2019-09-15T22:05:34.5770600Z     Checking syntax_pos v0.0.0 (/checkout/src/libsyntax_pos)
2019-09-15T22:05:36.0612747Z     Checking rustc_errors v0.0.0 (/checkout/src/librustc_errors)
2019-09-15T22:05:38.2195280Z     Checking fmt_macros v0.0.0 (/checkout/src/libfmt_macros)
2019-09-15T22:05:39.3539413Z error: unknown start of token: \u{a0}
2019-09-15T22:05:39.3540220Z    |
2019-09-15T22:05:39.3540220Z    |
2019-09-15T22:05:39.3540561Z 70 |     /// and `None` into `LhsExpr::NotYetParsed`.
2019-09-15T22:05:39.3540848Z    | ^
2019-09-15T22:05:39.3541211Z help: Unicode character ' ' (No-Break Space) looks like ' ' (Space), but it is not
2019-09-15T22:05:39.3541462Z    |
2019-09-15T22:05:39.3541817Z 70 |     /// and `None` into `LhsExpr::NotYetParsed`.
2019-09-15T22:05:39.3542181Z 
2019-09-15T22:05:45.1635390Z error: aborting due to previous error
2019-09-15T22:05:45.1636573Z 
2019-09-15T22:05:45.2143906Z error: Could not compile `syntax`.
---
2019-09-15T22:05:45.2238304Z == clock drift check ==
2019-09-15T22:05:45.2273095Z   local time: Sun Sep 15 22:05:45 UTC 2019
2019-09-15T22:05:45.5108326Z   network time: Sun, 15 Sep 2019 22:05:45 GMT
2019-09-15T22:05:45.5108496Z == end clock drift check ==
2019-09-15T22:05:46.6310476Z ##[error]Bash exited with code '1'.
2019-09-15T22:05:46.6345044Z ##[section]Starting: Checkout
2019-09-15T22:05:46.6347295Z ==============================================================================
2019-09-15T22:05:46.6347360Z Task         : Get sources
2019-09-15T22:05:46.6347425Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

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)

src/libsyntax/parse/parser/expr.rs

Centril · 2019-09-16T15:13:30Z

Can you squash the commits into one? r=me rollup when that's done.

Centril · 2019-09-19T16:46:23Z

@bors r+ rollup

bors · 2019-09-19T16:46:25Z

📌 Commit 194d357 has been approved by Centril

@skade

Document From trait for LhsExpr in parser Add doc for From trait for converting P<Expr> and Option<ThinVec<Attribute>> to LhsExpr As part of issue rust-lang#51430 (cc @skade). Both of these should just be moving an address and setting a discriminant in an enum. The main thing I'm not sure about is whether it's worth documenting the branch in the From<Option<ThinVec<Attribute>>. As far as I can tell it doesn't seem like it is optimized away (although if the discriminant happened to work out you could just copy the pointer and the discriminant which might be cheaper, but that's not guaranteed). So it seems like if it's being called often, it's doubling the number of possible branch mispredictions on this Option, which could be a significant cost. Let me know if there's anything that needs fixing and I'll get to it as soon as possible!

@ghost

Rollup of 8 pull requests Successful merges: - #64136 (Document From trait for LhsExpr in parser) - #64342 (factor out pluralisation remains after #64280) - #64387 (Fix redundant semicolon lint interaction with proc macro attributes) - #64498 (When possible point at argument causing item obligation failure) - #64615 (rustbuild: Turn down compression on exe installers) - #64617 (rustbuild: Turn down compression on msi installers) - #64618 (rustbuild: Improve output of `dist` step) - #64621 (Add Compatibility Notes to RELEASES.md for 1.38.0) Failed merges: r? @ghost

@skade

Document From trait for LhsExpr in parser Add doc for From trait for converting P<Expr> and Option<ThinVec<Attribute>> to LhsExpr As part of issue rust-lang#51430 (cc @skade). Both of these should just be moving an address and setting a discriminant in an enum. The main thing I'm not sure about is whether it's worth documenting the branch in the From<Option<ThinVec<Attribute>>. As far as I can tell it doesn't seem like it is optimized away (although if the discriminant happened to work out you could just copy the pointer and the discriminant which might be cheaper, but that's not guaranteed). So it seems like if it's being called often, it's doubling the number of possible branch mispredictions on this Option, which could be a significant cost. Let me know if there's anything that needs fixing and I'll get to it as soon as possible!

@skade

Document From trait for LhsExpr in parser Add doc for From trait for converting P<Expr> and Option<ThinVec<Attribute>> to LhsExpr As part of issue rust-lang#51430 (cc @skade). Both of these should just be moving an address and setting a discriminant in an enum. The main thing I'm not sure about is whether it's worth documenting the branch in the From<Option<ThinVec<Attribute>>. As far as I can tell it doesn't seem like it is optimized away (although if the discriminant happened to work out you could just copy the pointer and the discriminant which might be cheaper, but that's not guaranteed). So it seems like if it's being called often, it's doubling the number of possible branch mispredictions on this Option, which could be a significant cost. Let me know if there's anything that needs fixing and I'll get to it as soon as possible!

@skade

Document From trait for LhsExpr in parser Add doc for From trait for converting P<Expr> and Option<ThinVec<Attribute>> to LhsExpr As part of issue rust-lang#51430 (cc @skade). Both of these should just be moving an address and setting a discriminant in an enum. The main thing I'm not sure about is whether it's worth documenting the branch in the From<Option<ThinVec<Attribute>>. As far as I can tell it doesn't seem like it is optimized away (although if the discriminant happened to work out you could just copy the pointer and the discriminant which might be cheaper, but that's not guaranteed). So it seems like if it's being called often, it's doubling the number of possible branch mispredictions on this Option, which could be a significant cost. Let me know if there's anything that needs fixing and I'll get to it as soon as possible!

@skade

Document From trait for LhsExpr in parser Add doc for From trait for converting P<Expr> and Option<ThinVec<Attribute>> to LhsExpr As part of issue rust-lang#51430 (cc @skade). Both of these should just be moving an address and setting a discriminant in an enum. The main thing I'm not sure about is whether it's worth documenting the branch in the From<Option<ThinVec<Attribute>>. As far as I can tell it doesn't seem like it is optimized away (although if the discriminant happened to work out you could just copy the pointer and the discriminant which might be cheaper, but that's not guaranteed). So it seems like if it's being called often, it's doubling the number of possible branch mispredictions on this Option, which could be a significant cost. Let me know if there's anything that needs fixing and I'll get to it as soon as possible!

@skade

Document From trait for LhsExpr in parser Add doc for From trait for converting P<Expr> and Option<ThinVec<Attribute>> to LhsExpr As part of issue rust-lang#51430 (cc @skade). Both of these should just be moving an address and setting a discriminant in an enum. The main thing I'm not sure about is whether it's worth documenting the branch in the From<Option<ThinVec<Attribute>>. As far as I can tell it doesn't seem like it is optimized away (although if the discriminant happened to work out you could just copy the pointer and the discriminant which might be cheaper, but that's not guaranteed). So it seems like if it's being called often, it's doubling the number of possible branch mispredictions on this Option, which could be a significant cost. Let me know if there's anything that needs fixing and I'll get to it as soon as possible!

@skade

Document From trait for LhsExpr in parser Add doc for From trait for converting P<Expr> and Option<ThinVec<Attribute>> to LhsExpr As part of issue rust-lang#51430 (cc @skade). Both of these should just be moving an address and setting a discriminant in an enum. The main thing I'm not sure about is whether it's worth documenting the branch in the From<Option<ThinVec<Attribute>>. As far as I can tell it doesn't seem like it is optimized away (although if the discriminant happened to work out you could just copy the pointer and the discriminant which might be cheaper, but that's not guaranteed). So it seems like if it's being called often, it's doubling the number of possible branch mispredictions on this Option, which could be a significant cost. Let me know if there's anything that needs fixing and I'll get to it as soon as possible!

@skade

Document From trait for LhsExpr in parser Add doc for From trait for converting P<Expr> and Option<ThinVec<Attribute>> to LhsExpr As part of issue rust-lang#51430 (cc @skade). Both of these should just be moving an address and setting a discriminant in an enum. The main thing I'm not sure about is whether it's worth documenting the branch in the From<Option<ThinVec<Attribute>>. As far as I can tell it doesn't seem like it is optimized away (although if the discriminant happened to work out you could just copy the pointer and the discriminant which might be cheaper, but that's not guaranteed). So it seems like if it's being called often, it's doubling the number of possible branch mispredictions on this Option, which could be a significant cost. Let me know if there's anything that needs fixing and I'll get to it as soon as possible!

@skade

Document From trait for LhsExpr in parser Add doc for From trait for converting P<Expr> and Option<ThinVec<Attribute>> to LhsExpr As part of issue rust-lang#51430 (cc @skade). Both of these should just be moving an address and setting a discriminant in an enum. The main thing I'm not sure about is whether it's worth documenting the branch in the From<Option<ThinVec<Attribute>>. As far as I can tell it doesn't seem like it is optimized away (although if the discriminant happened to work out you could just copy the pointer and the discriminant which might be cheaper, but that's not guaranteed). So it seems like if it's being called often, it's doubling the number of possible branch mispredictions on this Option, which could be a significant cost. Let me know if there's anything that needs fixing and I'll get to it as soon as possible!

@ghost

Rollup of 9 pull requests Successful merges: - #64010 (Stabilize `param_attrs` in Rust 1.39.0) - #64136 (Document From trait for LhsExpr in parser) - #64342 (factor out pluralisation remains after #64280) - #64347 (Add long error explanation for E0312) - #64621 (Add Compatibility Notes to RELEASES.md for 1.38.0) - #64632 (remove the extra comma after the match arm) - #64640 (No home directory on vxWorks) - #64641 (Exempt extern "Rust" from improper_ctypes) - #64642 (Fix the span used to suggest avoiding for-loop moves) Failed merges: r? @ghost

bors · 2019-09-21T18:14:12Z

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

rust-highfive assigned eddyb Sep 3, 2019

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

Centril reviewed Sep 14, 2019

View reviewed changes

src/libsyntax/parse/parser/expr.rs Outdated Show resolved Hide resolved

src/libsyntax/parse/parser/expr.rs Outdated Show resolved Hide resolved

rust-highfive assigned Centril and unassigned eddyb Sep 14, 2019

Centril 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-review Status: Awaiting review from the assignee but also interested parties. labels Sep 14, 2019

crgl commented Sep 16, 2019

View reviewed changes

src/libsyntax/parse/parser/expr.rs Outdated Show resolved Hide resolved

Document From trait for LhsExpr

194d357

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 19, 2019

Centril mentioned this pull request Sep 20, 2019

Rollup of 8 pull requests #64626

Closed

Centril mentioned this pull request Sep 20, 2019

Rollup of 9 pull requests #64637

Closed

tmandry mentioned this pull request Sep 20, 2019

Rollup of 10 pull requests #64643

Closed

tmandry mentioned this pull request Sep 20, 2019

Rollup of 10 pull requests #64644

Closed

tmandry mentioned this pull request Sep 20, 2019

Rollup of 9 pull requests #64645

Closed

tmandry mentioned this pull request Sep 20, 2019

Rollup of 10 pull requests #64646

Closed

tmandry mentioned this pull request Sep 21, 2019

Rollup of 10 pull requests #64647

Closed

Centril mentioned this pull request Sep 21, 2019

Rollup of 7 pull requests #64657

Closed

Centril mentioned this pull request Sep 21, 2019

Rollup of 9 pull requests #64658

Merged

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 21, 2019

bors merged commit 194d357 into rust-lang:master Sep 21, 2019

crgl deleted the doc-from-parser-lhs branch October 2, 2019 01:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Document From trait for LhsExpr in parser #64136

Document From trait for LhsExpr in parser #64136

crgl commented Sep 3, 2019

rust-highfive commented Sep 3, 2019

rust-highfive commented Sep 3, 2019

rust-highfive commented Sep 4, 2019

JohnCSimon commented Sep 14, 2019

Centril commented Sep 14, 2019

rust-highfive commented Sep 15, 2019

Centril commented Sep 16, 2019

Centril commented Sep 19, 2019

bors commented Sep 19, 2019

bors commented Sep 21, 2019

Document From trait for LhsExpr in parser #64136

Document From trait for LhsExpr in parser #64136

Conversation

crgl commented Sep 3, 2019

rust-highfive commented Sep 3, 2019

rust-highfive commented Sep 3, 2019

rust-highfive commented Sep 4, 2019

JohnCSimon commented Sep 14, 2019

Centril commented Sep 14, 2019

rust-highfive commented Sep 15, 2019

Centril commented Sep 16, 2019

Centril commented Sep 19, 2019

bors commented Sep 19, 2019

bors commented Sep 21, 2019