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

Make Point Copy in arithmetic documentation #69766

Merged
merged 1 commit into from
Mar 11, 2020

Conversation

skade
Copy link
Contributor

@skade skade commented Mar 6, 2020

Small composite types like Point { x: i32, y: i32} are plain
old data and we should encourage users to derive Copy on them.

This changes the semantics of the edited examples slightly: instead
of consuming the operands during addition, it will copy them. This
is desired behaviour.

@rust-highfive
Copy link
Collaborator

r? @shepmaster

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 6, 2020
Copy link
Member

@shepmaster shepmaster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer the common order of these traits that I have seen.

src/libcore/ops/arith.rs Outdated Show resolved Hide resolved
src/libcore/ops/arith.rs Outdated Show resolved Hide resolved
src/libcore/ops/arith.rs Outdated Show resolved Hide resolved
src/libcore/ops/arith.rs Outdated Show resolved Hide resolved
src/libcore/ops/arith.rs Outdated Show resolved Hide resolved
src/libcore/ops/mod.rs Outdated Show resolved Hide resolved
@shepmaster
Copy link
Member

This changes the semantics of the edited examples slightly: instead
of consuming the operands during addition, it will copy them. This
is desired behaviour.

It's pure pedantry, but since the operands don't appear to be used after the operation (and they couldn't have been because they weren't Copy or Clone), there's no reason for them to be copied. They will still be moved into the method.

It comes down to the typical sci-fi dilemma: which of the two values is the copy and which is the original?

@rust-highfive
Copy link
Collaborator

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.
2020-03-10T10:59:33.6375270Z ========================== Starting Command Output ===========================
2020-03-10T10:59:33.6380701Z [command]/bin/bash --noprofile --norc /home/vsts/work/_temp/c04e70a7-0679-4035-bbec-3a3d41c60c5d.sh
2020-03-10T10:59:33.6381249Z 
2020-03-10T10:59:33.6385433Z ##[section]Finishing: Disable git automatic line ending conversion
2020-03-10T10:59:33.6407636Z ##[section]Starting: Checkout rust-lang/rust@refs/pull/69766/merge to s
2020-03-10T10:59:33.6412928Z Task         : Get sources
2020-03-10T10:59:33.6413542Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
2020-03-10T10:59:33.6413840Z Version      : 1.0.0
2020-03-10T10:59:33.6414033Z Author       : Microsoft
---
2020-03-10T10:59:36.4868952Z ##[command]git remote add origin https://github.com/rust-lang/rust
2020-03-10T10:59:36.4880706Z ##[command]git config gc.auto 0
2020-03-10T10:59:36.4889138Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2020-03-10T10:59:36.4896673Z ##[command]git config --get-all http.proxy
2020-03-10T10:59:36.4908383Z ##[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/69766/merge:refs/remotes/pull/69766/merge
---
2020-03-10T11:05:19.9528620Z    |
2020-03-10T11:05:19.9529184Z 46 | //! struct Point {
2020-03-10T11:05:19.9529812Z    | ^^^^^^^^^^^^^^^^^^
2020-03-10T11:05:19.9530286Z    |
2020-03-10T11:05:19.9531027Z    = note: inner doc comments like this (starting with `//!` or `/*!`) can only appear before items
2020-03-10T11:05:19.9537359Z error: aborting due to previous error
2020-03-10T11:05:19.9537613Z 
2020-03-10T11:05:19.9566643Z error: could not compile `core`.
2020-03-10T11:05:19.9575137Z warning: build failed, waiting for other jobs to finish...
---
2020-03-10T11:05:24.6932218Z   local time: Tue Mar 10 11:05:24 UTC 2020
2020-03-10T11:05:24.7851730Z   network time: Tue, 10 Mar 2020 11:05:24 GMT
2020-03-10T11:05:24.7852150Z == end clock drift check ==
2020-03-10T11:05:33.1545602Z 
2020-03-10T11:05:33.1630347Z ##[error]Bash exited with code '1'.
2020-03-10T11:05:33.1648617Z ##[section]Finishing: Run build
2020-03-10T11:05:33.1697408Z ##[section]Starting: Checkout rust-lang/rust@refs/pull/69766/merge to s
2020-03-10T11:05:33.1702686Z Task         : Get sources
2020-03-10T11:05:33.1703095Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
2020-03-10T11:05:33.1703480Z Version      : 1.0.0
2020-03-10T11:05:33.1703734Z Author       : Microsoft
2020-03-10T11:05:33.1703734Z Author       : Microsoft
2020-03-10T11:05:33.1704149Z Help         : [More Information](https://go.microsoft.com/fwlink/?LinkId=798199)
2020-03-10T11:05:33.1704647Z ==============================================================================
2020-03-10T11:05:33.5001438Z Cleaning any cached credential from repository: rust-lang/rust (GitHub)
2020-03-10T11:05:33.5050303Z ##[section]Finishing: Checkout rust-lang/rust@refs/pull/69766/merge to s
2020-03-10T11:05:33.5146396Z Cleaning up task key
2020-03-10T11:05:33.5147918Z Start cleaning up orphan processes.
2020-03-10T11:05:33.5344480Z Terminate orphan process: pid (4362) (python)
2020-03-10T11:05:33.5515276Z ##[section]Finishing: Finalize Job

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 @rust-lang/infra. (Feature Requests)

src/libcore/ops/mod.rs Outdated Show resolved Hide resolved
@skade
Copy link
Contributor Author

skade commented Mar 10, 2020

@shepmaster Interesting, I couldn't find a reference to that order somewhere. Most codebases I use put Clone before Copy. (prerequisite in front)

In any case, your order reads well and puts the Copy more upfront.

@shepmaster
Copy link
Member

Looks good to me; squash things together and we will merge!

Small composite types like `Point { x: i32, y: i32}` are plain
old data and we should encourage users to derive `Copy` on them.

This changes the semantics of the edited examples slightly: instead
of consuming the operands during addition, it will copy them. This
is desired behaviour.

Co-Authored-By: Jake Goulding <shepmaster@mac.com>
@skade skade force-pushed the make-point-copy-in-add-documentation branch from 6c24574 to 69aaed8 Compare March 10, 2020 17:12
@skade
Copy link
Contributor Author

skade commented Mar 10, 2020

@shepmaster thank you! Squashed!

@shepmaster
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Mar 10, 2020

📌 Commit 69aaed8 has been approved by shepmaster

@bors
Copy link
Contributor

bors commented Mar 10, 2020

🌲 The tree is currently closed for pull requests below priority 1000, this pull request will be tested once the tree is reopened

@bors 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 Mar 10, 2020
Centril added a commit to Centril/rust that referenced this pull request Mar 11, 2020
…ntation, r=shepmaster

Make Point `Copy` in arithmetic documentation

Small composite types like `Point { x: i32, y: i32}` are plain
old data and we should encourage users to derive `Copy` on them.

This changes the semantics of the edited examples slightly: instead
of consuming the operands during addition, it will copy them. This
is desired behaviour.
bors added a commit that referenced this pull request Mar 11, 2020
Rollup of 10 pull requests

Successful merges:

 - #66059 (mem::zeroed/uninit: panic on types that do not permit zero-initialization)
 - #69373 (Stabilize const for integer {to,from}_{be,le,ne}_bytes methods)
 - #69591 (Use TypeRelating for instantiating query responses)
 - #69625 (Implement nth, last, and count for iter::Copied)
 - #69645 (const forget tests)
 - #69766 (Make Point `Copy` in arithmetic documentation)
 - #69825 (make `mem::discriminant` const)
 - #69859 (fix #62456)
 - #69891 (Exhaustiveness checking, `Matrix::push`: recursively expand or-patterns)
 - #69896 (parse: Tweak the function parameter edition check)

Failed merges:

r? @ghost
@bors bors merged commit 452c147 into rust-lang:master Mar 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants