Skip to content

Make ast::Lifetime contain ast::Name instead of ast::ident #8016

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
wants to merge 1 commit into from

Conversation

lilac
Copy link
Contributor

@lilac lilac commented Jul 24, 2013

Fixes #7743. To make the compiler happy, I rewrite a use case of the "quote_item" macro. After we get a compiler snapshot that includes this change, the manual ast construction can be reverted back to using "quote_item".

let ext_cx = cx.ext_cx;

// The vector of test_descs for this crate
let test_descs = mk_test_descs(cx);

let tt = {
Copy link
Member

Choose a reason for hiding this comment

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

Wow, nice work! Could you put a note here, e.g.

// NOTE: Cannot use quote_item due to stage0 and #7743, revert after a snapshot

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great, I just want to add something like FIXME, but don't know how to express that uniformly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about this:
// FIXME: Cannot use lifetimes in quote_item due to stage0 and #7743, revert after a snapshot

Copy link
Member

Choose a reason for hiding this comment

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

That's fine, but it has to be a NOTE, or you have to file an issue, and write FIXME #nnnn ... (which is fine too). make tidy doesn't allow FIXME without an issue number.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, get it.

@emberian
Copy link
Member

@lilac in the future, when you update a PR, do r? @whoeverreviewedit to make sure it doesn't languish in the queue.

@lilac
Copy link
Contributor Author

lilac commented Jul 27, 2013

@cmr OK, thanks!

@huonw
Copy link
Member

huonw commented Jul 30, 2013

@lilac it looks like this failed with the same error as #8103 (i.e. make: *** [i686-pc-mingw32/stage1/bin/rustc/i686-pc-mingw32/bin/std.dll] Error 255), so if/when that gets fixed, this can probably go through too.

@jbclements
Copy link
Contributor

I'm interested, because afaict there's not actually any shared code between this and #8103 . FWIW, @graydon tells me that he reduced the failure to this code:

macro_rules! ok (($T:ident) => ($T))
macro_rules! bad (($T:ty)   => ($T))
//ok!(int);  // compiles
bad!(int); // crashes
fn main() { }

Unfortunately, I don't have a Windows machine to reproduce this failure. If you ( @lilac ) do, perhaps you can see whether this code triggers the failure on your PR, too?

Best,

John

@lilac
Copy link
Contributor Author

lilac commented Aug 1, 2013

@jbclements Sorry, I don't have a Windows machine neither. And I am quite surprised that two unrelated pieces of code result in the same error.

Maybe because the macro usage and my manual AST tree building both call some underlying AST related codes?

@lilac
Copy link
Contributor Author

lilac commented Aug 5, 2013

r? @huonw

@huonw
Copy link
Member

huonw commented Aug 5, 2013

Hm, @lilac, this doesn't merge cleanly, could you rebase on top of master?

@lilac
Copy link
Contributor Author

lilac commented Aug 5, 2013

@huonw Rebase done. Do I need to close this PR and create a new PR after rebase?

@huonw
Copy link
Member

huonw commented Aug 5, 2013

Nope, no need for a new PR, just force pushing the updated commits to this PR is fine.

@lilac
Copy link
Contributor Author

lilac commented Aug 5, 2013

r? @huonw

@lilac
Copy link
Contributor Author

lilac commented Aug 5, 2013

@bors: retry

@huonw
Copy link
Member

huonw commented Aug 5, 2013

(You need to be a reviewer to @bors: retry. :) )

We've accumulated a long chain of commits, could you squash them into the first one since it's all a single feature?

@lilac
Copy link
Contributor Author

lilac commented Aug 5, 2013

@huonw I still haven't known the reason for

it looks like this failed with the same error as #8103 (i.e. make: *** [i686-pc-mingw32/stage1/bin/rustc/i686-pc-mingw32/bin/std.dll] Error 255), so if/when that gets fixed, this can probably go through too.

Not sure if it worths a retry.

@catamorphism
Copy link
Contributor

@lilac Needs a rebase.

@catamorphism
Copy link
Contributor

Closing due to lack of activity. Please reopen if you have time to rebase it!

@jbclements
Copy link
Contributor

my patch went through, so this one might as well. I'll re-open it when I have time to do some baby-sitting.

flip1995 pushed a commit to flip1995/rust that referenced this pull request Dec 2, 2021
Add more descriptive help info for `needless_question_mark`

closes rust-lang#8016

changelog: [`needless_question_mark`] help suggestion now explains what should be changed
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.

easy fix: lifetimes should contain Name's, not idents
5 participants