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

macros: allow a path fragment to be parsed as a type parameter bound #38279

Merged
merged 1 commit into from
Dec 18, 2016

Conversation

KalitaAlexey
Copy link
Contributor

@KalitaAlexey KalitaAlexey commented Dec 10, 2016

Allow a path fragment to be parsed as a type parameter bound.
Fixes #8521.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nrc (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@KalitaAlexey
Copy link
Contributor Author

It has failed. Is it me?

@bluss
Copy link
Member

bluss commented Dec 10, 2016

It looks like the failure is not related to the PR, it ran all tests successfully.

Unfortunately all PRs fail on travis right now: https://travis-ci.org/rust-lang/rust/builds

@KalitaAlexey
Copy link
Contributor Author

Hi @bluss,

Thank you for the explanation.

Should I just wait or what?

Regards,
Alexey

@bluss
Copy link
Member

bluss commented Dec 10, 2016

Wait or maybe seek out and ask someone if they want to review your PR. It's not my area, so I will not review it.

@brson
Copy link
Contributor

brson commented Dec 11, 2016

@KalitaAlexey @nrc will review soon, but has been traveling lately.

@KalitaAlexey
Copy link
Contributor Author

Hi @brson,

I'll wait. It's not problem.

Regards,
Alexey

}
_ => break,
}
}
token::ModSep | token::Ident(..) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of adding the token::Interpolated(ref nt) => { ... } arm, I believe you could just change this pattern to

        _ if self.token.is_path_start() || self.token.is_keyword(keywords::For) => { ... }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll examine your suggestion.

trait Foo {}

macro_rules! foo(($t:path) => {
impl<T: $t> Foo for T {}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: indent 4 spaces

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll fix it.

foo!(m::m2::A);

fn main() {}

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: trailing newline (in both tests)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll fix it.

@jseyfried jseyfried assigned jseyfried and unassigned nrc Dec 12, 2016
@jseyfried jseyfried changed the title fixes #8521 macros: allow a path fragment to be parsed as a type parameter bound Dec 12, 2016
@jseyfried
Copy link
Contributor

cc @petrochenkov

@KalitaAlexey
Copy link
Contributor Author

Hi @jseyfried,
My commit somehow breaks src/test/run-pass/issue-28950.rs
I don't understand why.

Could you look at it?

Thanks

@jseyfried
Copy link
Contributor

  • What commit breaks run-pass/issue-28950.rs? The current PR or my suggestion?
  • In what way does it break? What is the error message?

@KalitaAlexey
Copy link
Contributor Author

@jseyfried,

I don't know why, but my PR enough to break the test.
I checked it before and it worked.
It should run and pass, but it crashes with segfault.

Thanks

@jseyfried
Copy link
Contributor

That sounds unrelated to this PR, but I don't have enough information to help debug further. Could you provide hash of the commit that breaks the test, the exact output when it crashes, and ideally a backtrace?

@KalitaAlexey
Copy link
Contributor Author

Hm. Actually my installed rustc also creates an executable failing with segfault.

@Mark-Simulacrum
Copy link
Member

@KalitaAlexey Can you attempt to provide a backtrace? Either way, please file a new issue (since this seems unrelated to this PR), with your OS distro and version, as well as rustc version so we can try to diagnose. I just verified locally, and it doesn't segfault for me.

@Mark-Simulacrum
Copy link
Member

Never mind. I had forgotten to run the produced executable, will post the new issue myself once I get a backtrace if you don't get to it first.

@Mark-Simulacrum
Copy link
Member

Oh. That tests currently doesn't support non-optimized codegen (no -O); @KalitaAlexey are you running without that for some reason?

@KalitaAlexey
Copy link
Contributor Author

Hi @Mark-Simulacrum,

I just ran ./x.py test.

@KalitaAlexey
Copy link
Contributor Author

Hi @jseyfried,

I applied your suggestions.

Thanks.

Copy link
Contributor

@jseyfried jseyfried left a comment

Choose a reason for hiding this comment

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

r=me modulo nit

foo2!(B<C>);

fn main() {}

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: trailing newline

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm. Right.

@@ -0,0 +1,35 @@
// Copyright 2014 The Rust Project Developers. See the COPYRIGHT
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: 2016, not 2014

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll fix it.

@KalitaAlexey
Copy link
Contributor Author

@jseyfried, I've done it.

@jseyfried
Copy link
Contributor

@KalitaAlexey Thanks!
One last thing -- could you squash the commits so that this PR is a single commit?

@KalitaAlexey
Copy link
Contributor Author

@jseyfried, how should I do it? I don't know right way.

@jseyfried
Copy link
Contributor

jseyfried commented Dec 16, 2016

cd rust
git checkout master
git checkout -b tmp
git merge issue-8521 --no-ff
git reset master
git add src
git commit -m "allow path fragments to be parsed as type parameter bounds"
git branch -D issue-8521
git branch -m issue-8521
git push origin issue-8521 --force

@KalitaAlexey
Copy link
Contributor Author

I've done what you had suggested me. Thank you.

@jseyfried
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Dec 16, 2016

📌 Commit 12a6cf1 has been approved by jseyfried

@bors
Copy link
Contributor

bors commented Dec 16, 2016

⌛ Testing commit 12a6cf1 with merge 9652757...

@bors
Copy link
Contributor

bors commented Dec 16, 2016

💔 Test failed - auto-win-msvc-64-opt

@jseyfried
Copy link
Contributor

@bors retry clean

@bors
Copy link
Contributor

bors commented Dec 16, 2016

⌛ Testing commit 12a6cf1 with merge bf9e5a8...

@bors
Copy link
Contributor

bors commented Dec 16, 2016

💔 Test failed - auto-win-msvc-64-opt

@alexcrichton
Copy link
Member

@bors: retry

@bors
Copy link
Contributor

bors commented Dec 17, 2016

⌛ Testing commit 12a6cf1 with merge ec8bb45...

bors added a commit that referenced this pull request Dec 17, 2016
macros: allow a `path` fragment to be parsed as a type parameter bound

Allow a `path` fragment to be parsed as a type parameter bound.
Fixes #8521.
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.

None yet

9 participants