Skip to content

MultiItemDecorator should take &Annotatable #25683

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

Closed
Manishearth opened this issue May 21, 2015 · 4 comments · Fixed by #25710
Closed

MultiItemDecorator should take &Annotatable #25683

Manishearth opened this issue May 21, 2015 · 4 comments · Fixed by #25710
Labels
A-plugins Area: compiler plugins, doc.rust-lang.org/nightly/unstable-book/language-features/plugin.html C-enhancement Category: An issue proposing an enhancement or a PR with one.

Comments

@Manishearth
Copy link
Member

pub trait ItemDecorator {
    fn expand(&self, ecx: &mut ExtCtxt, sp: Span, meta_item: &MetaItem, item: &Item, push: &mut FnMut(P<Item>));
}

is deprecated in favor of

pub trait MultiItemDecorator {
    fn expand(&self, ecx: &mut ExtCtxt, sp: Span, meta_item: &MetaItem, item: Annotatable, push: &mut FnMut(Annotatable));
}

The new version takes the thing being decorated by-move, and this is achieved by a clone. The clone is totally unnecessary; a decorator doesn't modify the original item and should be fine with a simple reference to it.

This probably causes a lot of unnecessary clones since MultiDecorators are used everywhere in the form of #[derive].

Should we make this &Annotatable and remove the clone?

cc @nrc @huonw

@Manishearth Manishearth added C-enhancement Category: An issue proposing an enhancement or a PR with one. A-plugins Area: compiler plugins, doc.rust-lang.org/nightly/unstable-book/language-features/plugin.html labels May 21, 2015
@bluss
Copy link
Member

bluss commented May 21, 2015

How large is annotatable? It only matters if it's expensive to clone.

@Manishearth
Copy link
Member Author

An Annotatable is generally an Item. In this case, mostly structs and enums. Cloning a struct in the AST isn't too expensive, but when you do it multiple times for almost every struct it adds up. I'm going to profile this for the heck of it and see.

@nrc
Copy link
Member

nrc commented May 21, 2015

An Annotatable is just an enum of pointers (a pointer to an item, not actually an item), so it is only one word, cloning it shouldn't make any difference. Think of it as a smart pointer. Still interested to see a profile.

@Manishearth
Copy link
Member Author

It contains P which does a deep clone

Manishearth added a commit to Manishearth/rust that referenced this issue May 22, 2015
oli-obk pushed a commit to oli-obk/rust that referenced this issue May 23, 2015
…fackler

fixes rust-lang#25683

I have a very nonscientific measurement of the data via valgrind/massif [here](https://gist.github.com/Manishearth/4c47f15f6835cb3957c4)

I measured the memory usage for both --pretty=expanded and -Z no-trans

It *seems* like there's a 20-25MB decrease during expansion on stage2 librustc; but I'm not quite sure.

r? @eddyb

(have not yet run tests, but it compiles fine, might want to wait before giving r+)

cc @nrc @huon
oli-obk pushed a commit to oli-obk/rust that referenced this issue May 23, 2015
oli-obk pushed a commit to oli-obk/rust that referenced this issue May 23, 2015
oli-obk pushed a commit to oli-obk/rust that referenced this issue May 23, 2015
…fackler

fixes rust-lang#25683

I have a very nonscientific measurement of the data via valgrind/massif [here](https://gist.github.com/Manishearth/4c47f15f6835cb3957c4)

I measured the memory usage for both --pretty=expanded and -Z no-trans

It *seems* like there's a 20-25MB decrease during expansion on stage2 librustc; but I'm not quite sure.

r? @eddyb

(have not yet run tests, but it compiles fine, might want to wait before giving r+)

cc @nrc @huon
oli-obk pushed a commit to oli-obk/rust that referenced this issue May 23, 2015
…fackler

fixes rust-lang#25683

I have a very nonscientific measurement of the data via valgrind/massif [here](https://gist.github.com/Manishearth/4c47f15f6835cb3957c4)

I measured the memory usage for both --pretty=expanded and -Z no-trans

It *seems* like there's a 20-25MB decrease during expansion on stage2 librustc; but I'm not quite sure.

r? @eddyb

(have not yet run tests, but it compiles fine, might want to wait before giving r+)

cc @nrc @huon
clatour added a commit to clatour/num that referenced this issue May 24, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-plugins Area: compiler plugins, doc.rust-lang.org/nightly/unstable-book/language-features/plugin.html C-enhancement Category: An issue proposing an enhancement or a PR with one.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants