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

Batch up libsyntax breaking changes #34424

Merged
merged 44 commits into from
Jun 28, 2016
Merged

Conversation

jseyfried
Copy link
Contributor

@jseyfried jseyfried commented Jun 23, 2016

Batch of the following syntax-[breaking-change] changes:

@Manishearth
Copy link
Member

I prefer having reviewer info stick around in the merge commit somehow. I have a process for this 😄

@Manishearth
Copy link
Member

you may also find this git alias useful for future stuff: fetchpr = !sh -c 'git fetch $1 pull/$2/head:pr-$2' -

@jseyfried
Copy link
Contributor Author

jseyfried commented Jun 23, 2016

Ok, didn't mean to disrupt your process. There were a lot of merge conflicts, mostly from my PRs, so I figured I'd take care of them. If you'd like to integrate that work from this branch into your process, feel free :)

@Manishearth
Copy link
Member

Manishearth commented Jun 23, 2016

Nah, it's fine. I'm okay with others doing it, just leave the reviewer info and PR text in there somehow. Especially if you've already fixed merge conflicts.

Could you use git rebase master --preserve-merges and just edit the commit text of the merge commits? I have code that does this already (not for rebases though), basically a mergerollup command in bash:

mergerollup() {
 echo "Merging $@"
 git branch -D current
 git fetch origin pull/$2/head:current
 NL="

"
 A=$(wget -O - https://api.github.com/repos/rust-lang/rust/pulls/$2 -q| grep "\"body\":"| sed  's/^  "body": "//'|sed 's/",$//'| sed 's/\\n/\n/g' | sed 's/\\r//g')

 git merge --no-ff current -m "Rollup merge of #$2 - $1, r=$3 $NL $A";
 git mergetool
}

which can be run using the code in #31645 (comment)

@jseyfried
Copy link
Contributor Author

jseyfried commented Jun 23, 2016

Will do.

@bors
Copy link
Contributor

bors commented Jun 23, 2016

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

@bors
Copy link
Contributor

bors commented Jun 27, 2016

📌 Commit 542ba8c has been approved by Manishearth

@sophiajt
Copy link
Contributor

@jseyfried - thanks :)

@bors
Copy link
Contributor

bors commented Jun 27, 2016

⌛ Testing commit 542ba8c with merge df7ef54...

@bors
Copy link
Contributor

bors commented Jun 27, 2016

💔 Test failed - auto-linux-64-opt-rustbuild

@jseyfried
Copy link
Contributor Author

@bors force r=Manishearth

@bors
Copy link
Contributor

bors commented Jun 27, 2016

📌 Commit 360dcae has been approved by Manishearth

@bors
Copy link
Contributor

bors commented Jun 27, 2016

⌛ Testing commit 360dcae with merge ea0dc92...

bors added a commit that referenced this pull request Jun 27, 2016
Batch up libsyntax breaking changes

Batch of the following syntax-[breaking-change] changes:
 - #34213: Add a variant `Macro` to `TraitItemKind`
 - #34368: Merge the variant `QPath` of `PatKind` into the variant `PatKind::Path`
 - #34385: Move `syntax::ast::TokenTree` into a new module `syntax::tokenstream`
 - #33943:
  - Remove the type parameter from `visit::Visitor`
  - Remove `attr::WithAttrs` -- use `attr::HasAttrs` instead.
  - Change `fold_tt`/`fold_tts` to take token trees by value and avoid wrapping token trees in `Rc`.
  - Remove the field `ctxt` of `ast::Mac_`
  - Remove inherent method `attrs()` of types -- use the method `attrs` of `HasAttrs` instead.
 - #34316:
  - Remove `ast::Decl`/`ast::DeclKind` and add variants `Local` and `Item` to `StmtKind`.
  - Move the node id for statements from the `StmtKind` variants to a field of `Stmt` (making `Stmt` a struct instead of an alias for `Spanned<StmtKind>`)
  - Rename `ast::ExprKind::Again` to `Continue`.
 - #34339: Generalize and abstract `ThinAttributes` to `ThinVec<Attribute>`
  - Use `.into()` in convert between `Vec<Attribute>` and `ThinVec<Attribute>`
  - Use autoderef instead of `.as_attr_slice()`
 - #34436: Remove the optional expression from `ast::Block` and instead use a `StmtKind::Expr` at the end of the statement list.
 - #34403: Move errors into a separate crate (unlikely to cause breakage)
@dtolnay
Copy link
Member

dtolnay commented Jul 2, 2016

That top comment is really helpful, thanks @jseyfried.

bors added a commit that referenced this pull request Jul 6, 2016
Fix expansion performance regression

**syntax-[breaking-change] cc #31645**

This fixes #34630 by reverting commit 5bf7970 of PR #33943, which landed in #34424.

By removing the `Rc<_>` wrapping around `Delimited` and `SequenceRepetition` in `TokenTree`, 5bf7970 made cloning `TokenTree`s more expensive. While this had no measurable performance impact on the compiler's crates, it caused an order of magnitude performance regression on some macro-heavy code in the wild. I believe this is due to clones of `TokenTree`s in `macro_parser.rs` and/or `macro_rules.rs`.

r? @nrc
bors added a commit that referenced this pull request Jul 7, 2016
Fix expansion performance regression

**syntax-[breaking-change] cc #31645**

This fixes #34630 by reverting commit 5bf7970 of PR #33943, which landed in #34424.

By removing the `Rc<_>` wrapping around `Delimited` and `SequenceRepetition` in `TokenTree`, 5bf7970 made cloning `TokenTree`s more expensive. While this had no measurable performance impact on the compiler's crates, it caused an order of magnitude performance regression on some macro-heavy code in the wild. I believe this is due to clones of `TokenTree`s in `macro_parser.rs` and/or `macro_rules.rs`.

r? @nrc
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.

7 participants