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.

Sign up for GitHub

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

Jump to bottom

librustc: Implement the Box<T> type syntax #13904

Merged
merged 1 commit into from
May 3, 2014
Merged

librustc: Implement the Box<T> type syntax #13904

merged 1 commit into from
May 3, 2014

Conversation

pcwalton
Copy link
Contributor

@pcwalton pcwalton commented May 2, 2014

r? @alexcrichton

RFC#14

Issue #13885.

@alexcrichton
Copy link
Member

I like this patch because it allows us to start porting code over ASAP. The biggest thing we gain is Box<T> where T is a type parameter like today, plus Box<Trait>.

I am worried about Box<str> and Box<[T]>. I don't think we want to migrate to either of these types, but rather Vec and Str. I also believe that this decision needs to go through a meeting (just to make sure we're all on the same page).

Could this patch be modified to deny Box<str> and Box<[T]>, while allowing Box<Trait> and Box<T> (where T is a normal type parameter today, not a dynamically sized one)? I think that then we'll have all the forward-compatible syntax that we plan on keeping and using forever, without having a weird interim where everything is Box<[T]> when it should rather be Vec<T>

alexcrichton
alexcrichton reviewed May 2, 2014
View reviewed changes
src/test/run-pass/new-box.rs
}

fn i(x: Box<Trait>) {
x.printme();
Copy link
Member

Choose a reason for hiding this comment

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

Could you also ensure that Box<Trait> can become &Trait?

@flaper87
Copy link
Contributor

flaper87 commented May 2, 2014

👍 glad to see this moving forward

@flaper87
Copy link
Contributor

flaper87 commented May 2, 2014

One thing, IIRC, there's an issue / RFC related to this change. Would you mind adding a reference to each in the commit message?

alexcrichton
alexcrichton reviewed May 2, 2014
View reviewed changes
src/librustc/middle/typeck/astconv.rs
ast::TyPath(ref path, _, id) => {
let a_def = match this.tcx().def_map.borrow().find(&id) {
None => this.tcx().sess.span_fatal(
ast_ty.span, format!("unbound path {}", path_to_str(path))),
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be span_bug

@alexcrichton
Copy link
Member

r=me with a few nits

bors added a commit that referenced this pull request May 3, 2014
@bors
auto merge of #13904 : pcwalton/rust/box, r=alexcrichton
bca9647 
r? @alexcrichton

RFC#14 

Issue #13885.
@bors bors closed this May 3, 2014
@bors bors merged commit 7c64f03 into rust-lang:master May 3, 2014
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 17, 2024
@bors
Auto merge of rust-lang#16789 - wyatt-herkamp:cfg_attr, r=Veykril
d3e6fea 
cfg Attribute Stripping for Proc Macro Expansion

This will attempt to process cfg attributes and cfg_attr attributes for proc macro expansion.
![image](https://github.com/rust-lang/rust-analyzer/assets/11785959/b85ef203-14a5-44c9-9b67-59a65a5f2d96)

Closes rust-lang#8434 , rust-lang#11657, and rust-lang#13904
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pcwalton @alexcrichton @flaper87 @bors