Add the identity function as core::convert::identity #47562

Centril · 2018-01-19T02:37:01Z

New notes

This implements rust-lang/rfcs#2306 (see #53500).

Old notes (ignore this in new reviews)

Adds the identity function fn id<T>(x: T) -> T { x } to core::convert and the prelude.
Some motivations for why this is useful are explained in the doc tests.
Another is that using the identity function instead of { x } or |x| x makes it clear that you intended to use an identity conversion on purpose.

The reasoning:

behind adding this to convert and not mem is that this is an identity conversion.
for adding this to the prelude is that it should be easy enough to use that the ease of writing your own identity function or using a closure |x| x doesn't overtake that.

I've separated this out into two feature gates so that the addition to the prelude can be considered and stabilized separately.

cc @bluss

rust-highfive · 2018-01-19T02:37:13Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @sfackler (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.

Manishearth · 2018-01-19T07:52:45Z

I'm wary of adding unstable stuff to the prelude, feels like it could still cause clashes with other glob imports.

(also, shouldn't this go through an rfc first?)

Centril · 2018-01-19T08:04:20Z

I'm wary of adding unstable stuff to the prelude, feels like it could still cause clashes with other glob imports.

Any example you have in mind? AFAIK any import or function definition that the user makes, glob or not will override anything in the prelude..

(also, shouldn't this go through an rfc first?)

I can do that if you want...

But the addition is rather minor - there's not much technical to discuss - only if we want this in libcore or not... And there's precedent for this in other languages like Haskell, Scala, Java, Idris

Manishearth · 2018-01-19T08:06:31Z

@Centril the interaction between two glob imports is the thing here.

Adding stuff to the stdlib usually goes through an RFC, however trivial. This goes double for things in the prelude. It's sometimes fine to add stuff as a feature-gated API before making the RFC but you need some justification for that.

Centril · 2018-01-19T08:08:15Z

@Manishearth Alright; I'll write up an RFC. I'll leave this PR open meanwhile so that we can use it if the RFC gets accepted.

Manishearth · 2018-01-19T08:11:31Z

Probably should close this and reopen if the RFC merges. Unless we intend to get it merged soon, it shouldn't stay open.

Centril · 2018-01-19T08:19:56Z

That's fair. Expect and RFC filed by the end of the day.

bluss · 2018-01-19T11:55:55Z

The prelude change seems good to have an rfc for? New minor features do not otherwise need an rfc, I think this change fits well inside what has been added as unstable without rfc recently.

Centril · 2018-01-19T12:21:43Z

@bluss Yeah. Tho I think that in this case the function is much less useful outside the prelude.

Centril · 2018-08-19T19:13:05Z

Once the tests pass, this should be ready to go.

I've added tests for the rustc_const_unstable(..) bit as well.

varkor · 2018-08-20T01:30:41Z

src/libcore/convert.rs

+/// let filtered = iter.filter_map(identity).collect::<Vec<_>>();
+/// assert_eq!(vec![1, 3], filtered);
+/// ```
+#[unstable(feature = "convert_id", issue = "0")]


Should be issue = "53500" now.

Cheers; you always miss something :D

oli-obk · 2018-08-20T09:04:19Z

@bors r+

bors · 2018-08-20T09:04:20Z

📌 Commit 86641d9 has been approved by oli-obk

bors · 2018-08-20T09:10:02Z

⌛ Testing commit 86641d9 with merge bf1e461...

@bluss

Add the identity function as core::convert::identity ## New notes This implements rust-lang/rfcs#2306 (see #53500). ## Old notes (ignore this in new reviews) Adds the identity function `fn id<T>(x: T) -> T { x }` to core::convert and the prelude. Some motivations for why this is useful are explained in the doc tests. Another is that using the identity function instead of `{ x }` or `|x| x` makes it clear that you intended to use an identity conversion on purpose. The reasoning: + behind adding this to `convert` and not `mem` is that this is an identity *conversion*. + for adding this to the prelude is that it should be easy enough to use that the ease of writing your own identity function or using a closure `|x| x` doesn't overtake that. I've separated this out into two feature gates so that the addition to the prelude can be considered and stabilized separately. cc @bluss

bors · 2018-08-20T11:45:40Z

☀️ Test successful - status-appveyor, status-travis
Approved by: oli-obk
Pushing bf1e461 to master...

Centril added 5 commits January 19, 2018 01:27

add fn core::convert::id<T>(x: T) -> T { x }

351fefb

add fn core::convert::id<T>(x: T) -> T { x } to the prelude

651b25a

fix prelude reexport of convert::id

567f040

add #![feature(convert_id)] to libstd + libcore

93debaa

fix doctests for convert::id

ac64ef3

rust-highfive assigned sfackler Jan 19, 2018

kennytm added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 19, 2018

Manishearth closed this Jan 19, 2018

Centril mentioned this pull request Jan 19, 2018

RFC: Add pub fn identity<T>(x: T) -> T { x } to core::convert rust-lang/rfcs#2306

Merged

Centril changed the title ~~Add the identity function as core::convert::id (+ to prelude)~~ Add the identity function as core::convert::identity Aug 19, 2018

Centril reopened this Aug 19, 2018

Centril changed the title ~~Add the identity function as core::convert::identity~~ [WIP] Add the identity function as core::convert::identity Aug 19, 2018

Centril added 6 commits August 19, 2018 18:34

Merge branch 'master' into feature/core_convert_id

08b1d83

Drop identity from prelude.

8208c77

Get rid of unnecessary #![feature(core_float)].

c2217b7

Make core::convert::identity a const fn.

71187b7

remove feature(convert_id) from lib{core,std}/lib.rs

f84ec02

convert-id: tests for const gating.

b7772e6

Centril changed the title ~~[WIP] Add the identity function as core::convert::identity~~ Add the identity function as core::convert::identity Aug 19, 2018

varkor reviewed Aug 20, 2018

View reviewed changes

core::convert::identity: fix issue number to rust-lang#53500

86641d9

Centril mentioned this pull request Aug 20, 2018

Make LocalWaker::new a const fn #53456

Closed

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 Aug 20, 2018

bors merged commit 86641d9 into rust-lang:master Aug 20, 2018

Centril deleted the feature/core_convert_id branch August 20, 2018 11:46

Centril mentioned this pull request Oct 23, 2018

Tracking issue for RFC 2306, "Add core::convert::identity" #53500

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the identity function as core::convert::identity #47562

Add the identity function as core::convert::identity #47562

Centril commented Jan 19, 2018 •

edited

Loading

rust-highfive commented Jan 19, 2018

Manishearth commented Jan 19, 2018

Centril commented Jan 19, 2018

Manishearth commented Jan 19, 2018

Centril commented Jan 19, 2018 •

edited

Loading

Manishearth commented Jan 19, 2018

Centril commented Jan 19, 2018

bluss commented Jan 19, 2018

Centril commented Jan 19, 2018

Centril commented Aug 19, 2018

varkor Aug 20, 2018

Centril Aug 20, 2018

oli-obk commented Aug 20, 2018

bors commented Aug 20, 2018

bors commented Aug 20, 2018

bors commented Aug 20, 2018

Add the identity function as core::convert::identity #47562

Add the identity function as core::convert::identity #47562

Conversation

Centril commented Jan 19, 2018 • edited Loading

New notes

Old notes (ignore this in new reviews)

rust-highfive commented Jan 19, 2018

Manishearth commented Jan 19, 2018

Centril commented Jan 19, 2018

Manishearth commented Jan 19, 2018

Centril commented Jan 19, 2018 • edited Loading

Manishearth commented Jan 19, 2018

Centril commented Jan 19, 2018

bluss commented Jan 19, 2018

Centril commented Jan 19, 2018

Centril commented Aug 19, 2018

varkor Aug 20, 2018

Choose a reason for hiding this comment

Centril Aug 20, 2018

Choose a reason for hiding this comment

oli-obk commented Aug 20, 2018

bors commented Aug 20, 2018

bors commented Aug 20, 2018

bors commented Aug 20, 2018

Centril commented Jan 19, 2018 •

edited

Loading

Centril commented Jan 19, 2018 •

edited

Loading