Skip to content

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

Closed
lilac wants to merge 1 commit into
rust-lang:masterfrom
lilac:lifetime-name
Closed

Make ast::Lifetime contain ast::Name instead of ast::ident#8016
lilac wants to merge 1 commit into
rust-lang:masterfrom
lilac:lifetime-name

Conversation

@lilac

Copy link
Copy Markdown
Contributor

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".

Comment threadsrc/librustc/front/test.rs Outdated

Copy link
Copy Markdown
Contributor

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
Copy Markdown
ContributorAuthor

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
Copy Markdown
ContributorAuthor

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
Copy Markdown
Contributor

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
Copy Markdown
ContributorAuthor

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
Copy Markdown
Contributor

@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
Copy Markdown
ContributorAuthor

@cmr OK, thanks!

@huonw

Copy link
Copy Markdown
Contributor

@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
Copy Markdown
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

lilac commented Aug 1, 2013

Copy link
Copy Markdown
ContributorAuthor

@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

lilac commented Aug 5, 2013

Copy link
Copy Markdown
ContributorAuthor

r? @huonw

@huonw

huonw commented Aug 5, 2013

Copy link
Copy Markdown
Contributor

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

@lilac

lilac commented Aug 5, 2013

Copy link
Copy Markdown
ContributorAuthor

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

@huonw

huonw commented Aug 5, 2013

Copy link
Copy Markdown
Contributor

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

@lilac

lilac commented Aug 5, 2013

Copy link
Copy Markdown
ContributorAuthor

r? @huonw

@lilac

lilac commented Aug 5, 2013

Copy link
Copy Markdown
ContributorAuthor

@bors: retry

@huonw

huonw commented Aug 5, 2013

Copy link
Copy Markdown
Contributor

(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

lilac commented Aug 5, 2013

Copy link
Copy Markdown
ContributorAuthor

@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
Copy Markdown
Contributor

@lilac Needs a rebase.

@catamorphism

Copy link
Copy Markdown
Contributor

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

@jbclements

Copy link
Copy Markdown
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`
closesrust-lang#8016
changelog: [`needless_question_mark`] help suggestion now explains what should be changed
U007D pushed a commit to U007D/rust-mos that referenced this pull request Aug 21, 2026
8016: More Chalk adaptations r=flodiebold a=flodiebold
- rename a bunch of fields
- use `chalk_ir::FnSig`
Co-authored-by: Florian Diebold <flodiebold@gmail.com>
Sign up for freeto 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

@lilac@emberian@huonw@jbclements@catamorphism