Refactor the way methods are stored in ast::ImplItem/TraitItem. #23265
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
eddyb
force-pushed
the
meth-ast-refactor
branch
from
caae538
to
ad05505
Compare
| @@ -13,7 +13,7 @@ fn main() { | |||
| pub struct A; //~ ERROR: visibility has no effect | |||
| pub enum B {} //~ ERROR: visibility has no effect | |||
| pub trait C { //~ ERROR: visibility has no effect | |||
| pub fn foo(&self) {} //~ ERROR: visibility has no effect | |||
| fn foo(&self) {} | |||
There was a problem hiding this comment.
Why remove these privacy tests - are they no longer checked - that seems bad
There was a problem hiding this comment.
They are parse errors now - I guess I should move them to parse-fail tests.
There was a problem hiding this comment.
Ok, I think it's a good idea to move the tests rather than removing
There was a problem hiding this comment.
I've added two parse-fail tests. I hope they're enough.
eddyb
force-pushed
the
meth-ast-refactor
branch
2 times, most recently
from
f608bae
to
def8023
Compare
|
LGTM, r+, but I'll leave it for @nikomatsakis to make it official in case he has any thoughts about the AST design. |
eddyb
force-pushed
the
meth-ast-refactor
branch
from
def8023
to
4fdacbb
Compare
…thod into MethodTraitItem.
eddyb
force-pushed
the
meth-ast-refactor
branch
from
4fdacbb
to
9da9185
Compare
|
I've been wanting to do similar cleanup and consolidation forever. Nice. |
bors
added a commit
that referenced
this pull request
Mar 12, 2015
The end result is that common fields (id, name, attributes, etc.) are stored in now-structures `ImplItem` and `TraitItem`. The signature of a method is no longer duplicated between methods with a body (default/impl) and those without, they now share `MethodSig`. This is also a [breaking-change] because of minor bugfixes and changes to syntax extensions: * `pub fn` methods in a trait no longer parse - remove the `pub`, it has no meaning anymore * `MacResult::make_methods` is now `make_impl_items` and the return type has changed accordingly * `quote_method` is gone, because `P<ast::Method>` doesn't exist and it couldn't represent a full method anyways - could be replaced by `quote_impl_item`/`quote_trait_item` in the future, but I do hope we realize how silly that combinatorial macro expansion is and settle on a single `quote` macro + some type hints - or just no types at all (only token-trees) r? @nikomatsakis This is necessary (hopefully also sufficient) for associated constants.
This was referenced Mar 12, 2015
Manishearth
pushed a commit
to Manishearth/rust
that referenced
this pull request
Mar 20, 2015
This fixes several use cases that were broken after rust-lang#23265 landed.
Manishearth
added a commit
to Manishearth/rust
that referenced
this pull request
Mar 20, 2015
This fixes several use cases that were broken after rust-lang#23265 landed.
This was referenced Apr 2, 2015
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The end result is that common fields (id, name, attributes, etc.) are stored in now-structures
ImplItemandTraitItem.The signature of a method is no longer duplicated between methods with a body (default/impl) and those without, they now share
MethodSig.This is also a [breaking-change] because of minor bugfixes and changes to syntax extensions:
pub fnmethods in a trait no longer parse - remove thepub, it has no meaning anymoreMacResult::make_methodsis nowmake_impl_itemsand the return type has changed accordinglyquote_methodis gone, becauseP<ast::Method>doesn't exist and it couldn't represent a full method anyways - could be replaced byquote_impl_item/quote_trait_itemin the future, but I do hope we realize how silly that combinatorial macro expansion is and settle on a singlequotemacro + some type hints - or just no types at all (only token-trees)r? @nikomatsakis This is necessary (hopefully also sufficient) for associated constants.