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

Give Names to positional fields and merge them with named fields #31937

Merged
merged 4 commits into from
Mar 2, 2016

Conversation

petrochenkov
Copy link
Contributor

The names are "0", "1", "2" etc, the same as used in field access.

This generally make things simpler and potentially allows to reuse braced struct machinery (struct patterns, struct expressions) for tuple structs.

I haven't touched the AST for stability reasons, but I intend to do it later.

r? @eddyb

@@ -197,8 +195,8 @@ fn build_struct(cx: &DocContext, tcx: &ty::ctxt, did: DefId) -> clean::Struct {
clean::Struct {
struct_type: match &*variant.fields {
[] => doctree::Unit,
[ref f] if f.name == unnamed_field.name => doctree::Newtype,
[ref f, ..] if f.name == unnamed_field.name => doctree::Tuple,
[_] if variant.kind == ty::VariantKind::Tuple => doctree::Newtype,
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this even matters anymore.

@eddyb
Copy link
Member

eddyb commented Feb 27, 2016

LGTM, but maybe @nrc or @Manishearth have more to say? If not, I'll r+ this later.

@Manishearth
Copy link
Member

I don't think the AST should be touched at all in this case; from a syntax extension perspective having positional fields separated is helpful; and it's not harming much.

As far as reusing machinery is concerned, .fields() is already pretty good.

@Manishearth
Copy link
Member

actual change lgtm

@apasel422
Copy link
Contributor

Does this fix #27340?

@petrochenkov
Copy link
Contributor Author

@apasel422

Does this fix #27340?

Indeed, it does. I've added a test for it.

@bors
Copy link
Contributor

bors commented Mar 2, 2016

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

@petrochenkov
Copy link
Contributor Author

Rebased.
ping @eddyb

@eddyb
Copy link
Member

eddyb commented Mar 2, 2016

@bors r+

@bors
Copy link
Contributor

bors commented Mar 2, 2016

📌 Commit bc93fbc has been approved by eddyb

@bors
Copy link
Contributor

bors commented Mar 2, 2016

⌛ Testing commit bc93fbc with merge 25e42ac...

bors added a commit that referenced this pull request Mar 2, 2016
The names are "0", "1", "2" etc, the same as used in field access.

This generally make things simpler and potentially allows to reuse braced struct machinery (struct patterns, struct expressions) for tuple structs.

I haven't touched the AST for stability reasons, but I intend to do it later.

r? @eddyb
@bors bors merged commit bc93fbc into rust-lang:master Mar 2, 2016
Manishearth added a commit to Manishearth/rust that referenced this pull request Apr 6, 2016
 The AST part of rust-lang#31937

Unlike HIR, AST still uses `Option` for field names because parser can't know field indexes reliably due to constructions like
```
struct S(#[cfg(false)] u8, u8); // The index of the second field changes from 1 during parsing to 0 after expansion.
```
and I wouldn't like to put the burden of renaming fields on expansion passes and syntax extensions.

plugin-[breaking-change] cc rust-lang#31645
r? @Manishearth
@petrochenkov petrochenkov deleted the field branch September 21, 2016 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants