Allow syntax extensions to return multiple items, closes #16723 #17236

fhahn · 2014-09-13T17:39:52Z

This is PR is basically the change proposed by @SimonSapin for #16723. I can also extend MacItem, as @huonw suggested, but I'm not sure how that was meant exactly. A comment for MacItem states that it's a convenience type for macros that return a single item. Should I adapt MacItem to contain a SmallVector of items instead of a single item and update the comment?

And I've two questions concerning the patch.

First, I am not sure what's the best way to duplicate the SmallVector holding the items. The way I'm doing it at the moment strikes me as suboptimal (https://github.com/fhahn/rust/compare/issue-16723-multiple-items?expand=1#diff-5100da09ad52c81568562e36ee359343R241). An additional map() call is required, because iter() returns an iterator to pointers to the elements. Is there a better way? Should I open a ticket and implement clone() for SmallVector?

And second, I had problems running the test in the test suite because it complains that there is no name MacItems in syntax::base::ext although there is. When I compile the file with

 LD_LIBRARY_PATH=x86_64-unknown-linux-gnu/stage2/lib/ 
 x86_64-unknown-linux-gnu/stage1/lib/rustlib/x86_64-unknown-linux-gnu/bin/rustc 
 -L x86_64-unknown-linux-gnu/stage1/lib/rustlib/x86_64-unknown-linux-gnu/lib/ 
 src/test/auxiliary/issue_16723_multiple_items_syntax_ext.rs

it works. But I'm not sure which rustc binary should be used, e.g. there is on stage1 build in x86_64-unknown-linux-gnu/stage1/bin/rustc and one in x86_64-unknown-linux-gnu/stage1/lib/rustlib/x86_64-unknown-linux-gnu/bin/rustc.

alexcrichton · 2014-09-15T17:43:10Z

src/libsyntax/ext/base.rs

+impl MacResult for MacItems {
+    fn make_items(self: Box<MacItems>) -> Option<SmallVector<P<ast::Item>>> {
+        // FIXME: Implement Clone on SmallVector and use it instead of .iter().collect()
+        Some(self.items.as_slice().iter().map(|x| x.clone()).collect())


This takes self by value now so this could just be Some(self.items.move_iter().collect())

Or just Some(self.items)

Now that MacItems does indeed store a SmallVector this should just return Some(self.items)

Ah, yes! Updated the PR.

alexcrichton · 2014-09-15T17:43:22Z

cc @sfackler

sfackler · 2014-09-15T17:45:37Z

You might as well remove MacItem entirely.

sfackler · 2014-09-15T17:47:15Z

src/test/run-pass/issue_16723_multiple_items_syntax_ext.rs

+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+// aux-build:issue_16723_multiple_items_syntax_ext.rs


This needs to have an // ignore-stage1.

fhahn · 2014-09-15T20:42:51Z

Thanks for your feedback, I've updated the PR.

alexcrichton · 2014-09-17T16:54:01Z

src/libsyntax/ext/base.rs

+
+impl MacResult for MacItems {
+    fn make_items(self: Box<MacItems>) -> Option<SmallVector<P<ast::Item>>> {
+        // FIXME: Implement Clone on SmallVector and use it instead of .iter().collect()


This FIXME can likely be removed now

Yup, thanks. Does it still make sense to implement Clone on SmallVector?

sfackler · 2014-09-18T22:29:26Z

src/test/run-pass/issue_16723_multiple_items_syntax_ext.rs

@@ -0,0 +1,30 @@
+// Copyright 2012 The Rust Project Developers. See the COPYRIGHT


This needs to go in run_pass_fulldeps

Thanks, that was the problem!

The test should pass now, thanks for your help and patience ;)

. This patch replaces `MacItem` with `MacItems`.

fhahn force-pushed the issue-16723-multiple-items branch 3 times, most recently from 79f55c7 to 5f0c2ea Compare September 14, 2014 13:26

alexcrichton reviewed Sep 15, 2014
View reviewed changes

sfackler reviewed Sep 15, 2014
View reviewed changes

fhahn force-pushed the issue-16723-multiple-items branch from 5f0c2ea to 36b3079 Compare September 15, 2014 20:42

fhahn force-pushed the issue-16723-multiple-items branch from 36b3079 to 98d6501 Compare September 17, 2014 16:27

alexcrichton reviewed Sep 17, 2014
View reviewed changes

fhahn force-pushed the issue-16723-multiple-items branch from 98d6501 to f694386 Compare September 17, 2014 20:46

fhahn force-pushed the issue-16723-multiple-items branch from f694386 to 3fc46ea Compare September 18, 2014 09:00

sfackler reviewed Sep 18, 2014
View reviewed changes

Allow syntax extensions to return multiple items, closes rust-lang#16723

89b0944

. This patch replaces `MacItem` with `MacItems`.

fhahn force-pushed the issue-16723-multiple-items branch from 3fc46ea to 89b0944 Compare September 19, 2014 09:11

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Sep 19, 2014

rollup merge of rust-lang#17236 : fhahn/issue-16723-multiple-items

1686353

bors merged commit 89b0944 into rust-lang:master Sep 20, 2014

fhahn deleted the issue-16723-multiple-items branch September 20, 2014 19:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow syntax extensions to return multiple items, closes #16723 #17236

Allow syntax extensions to return multiple items, closes #16723 #17236

fhahn commented Sep 13, 2014

alexcrichton Sep 15, 2014

SimonSapin Sep 15, 2014

alexcrichton Sep 17, 2014

fhahn Sep 17, 2014

alexcrichton commented Sep 15, 2014

sfackler commented Sep 15, 2014

sfackler Sep 15, 2014

fhahn commented Sep 15, 2014

alexcrichton Sep 17, 2014

fhahn Sep 17, 2014

sfackler Sep 18, 2014

fhahn Sep 19, 2014

		@@ -0,0 +1,30 @@
		// Copyright 2012 The Rust Project Developers. See the COPYRIGHT

Allow syntax extensions to return multiple items, closes #16723 #17236

Allow syntax extensions to return multiple items, closes #16723 #17236

Conversation

fhahn commented Sep 13, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Sep 15, 2014

sfackler commented Sep 15, 2014

Choose a reason for hiding this comment

fhahn commented Sep 15, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment