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

Move Bound to libcore. #42268

Closed
wants to merge 1 commit into from
Closed

Conversation

clarfonthey
Copy link
Contributor

Half of a redone version of #41460. This creates a new module, core::collections, which currently only contains this type.

@rust-highfive
Copy link
Collaborator

r? @alexcrichton

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

@Mark-Simulacrum Mark-Simulacrum added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 28, 2017
@Mark-Simulacrum
Copy link
Member

Looks like travis failed:

[00:01:36] error: This node does not have a stability attribute
[00:01:36]   --> /checkout/src/libcore/collections.rs:48:14
[00:01:36]    |
[00:01:36] 48 |     Included(T),
[00:01:36]    |              ^^
[00:01:36] 
[00:01:36] error: This node does not have a stability attribute
[00:01:36]   --> /checkout/src/libcore/collections.rs:51:14
[00:01:36]    |
[00:01:36] 51 |     Excluded(T),
[00:01:36]    |              ^^
[00:01:36] 
[00:01:37] error: aborting due to 2 previous errors

@clarfonthey
Copy link
Contributor Author

Indeed it did. I'm not quite sure why, though, because those nodes do have stability attributes.

@alexcrichton
Copy link
Member

I'm looking around and trying to find the rationale for this (I've forgotten at this point), can you remind me the reason for this move?

@clarfonthey
Copy link
Contributor Author

@alexcrichton this is basically a prerequisite for moving RangeArgument to core

@alexcrichton
Copy link
Member

Hm sorry but forgive my ignorance, why do we want to do that?

@clarfonthey
Copy link
Contributor Author

@alexcrichton considering how the Range types are all in core, it makes sense to move these to libcore as well. I mentioned it in the tracking issue and people seemed to agree.

@alexcrichton
Copy link
Member

Hm so we don't tend to move types around "just because", can you elaborate on the rationale for moving these types into core? Will this enable more use cases?

@clarfonthey
Copy link
Contributor Author

I mean, it gives you the ability to operate on RangeArgument instead of discrete range types for any no_std crate. So, I'd consider that a big plus.

no_std collections that work on the stack would be a big use case.

@clarfonthey clarfonthey force-pushed the bound_core branch 2 times, most recently from 3d9f4d7 to 69a450c Compare June 3, 2017 03:46
@clarfonthey
Copy link
Contributor Author

Also I added stability markers to the T and it compiles now?? I'm not sure why it needs this but I'll take it.

@clarfonthey clarfonthey force-pushed the bound_core branch 2 times, most recently from da06f57 to 9003062 Compare June 3, 2017 04:28
@Mark-Simulacrum
Copy link
Member

Link checker found a few problems.

[00:53:08] Linkcheck (x86_64-unknown-linux-gnu)
[00:53:13] core/collections/index.html:54: broken link - core/std/collections/index.html
[00:53:13] core/collections/enum.Bound.html:71: broken link - core/collections/btree_map/struct.BTreeMap.html
[00:53:14] thread 'main' panicked at 'found some broken links', /checkout/src/tools/linkchecker/main.rs:49

/// An inclusive bound.
#[stable(feature = "collections_bound", since = "1.17.0")]
Included(
#[stable(feature = "collections_bound", since = "1.17.0")] // ???
Copy link
Member

Choose a reason for hiding this comment

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

Can you remove these // ??? annotations? This is just how the stability check works.

@alexcrichton
Copy link
Member

I personally feel like we're not quite ready to do this yet. This seems to put us into a sort of "half baked" state where we have a stable collections module in libcore with no items other than Bound. It's ripe for expansion but we tend to prefer halfway states when committing to the master branch. Can this be delayed until there's more types/traits to move into the module?

@clarfonthey
Copy link
Contributor Author

Sounds good to me!

Also the reason why I added the ???s was because they weren't required on the enum before I moved it. I've never seen the linter require stability attributes for the inside of tuple variants/structs before.

@clarfonthey
Copy link
Contributor Author

Closing and moving the discussion to #30877.

@clarfonthey clarfonthey closed this Jun 8, 2017
@clarfonthey clarfonthey deleted the bound_core branch April 15, 2018 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants