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

ICH: Handle MacroDef HIR instances. #37787

Merged
merged 5 commits into from
Nov 19, 2016

Conversation

michaelwoerister
Copy link
Member

As of recently, hir::MacroDef instances are exported in crate metadata, which means we also store their ICH when doing incremental compilation. Even though exported macro definitions should not (yet) interact with incremental compilation, the ICH is also used for the general purpose crate hash, where macros should be included.

This PR implements ICH computation for MacroDef. In theory, the ICH of these MacroDefs is less stable than that of other HIR items, since I opted to just call the compiler-generated Hash::hash() for Token::Interpolated variants. Token::Interpolated contains AST data structures and it would have been a lot of effort to expand ICH computation to the AST too. Since quasi-quoting is rarely used and it would only make a difference if incremental compilation was extended to macros, the simpler implementation seemed like a good idea.

This fixes the problem reported in #37756. The test still fails because of broken codegen-unit support though.

r? @nikomatsakis

// Since this is hardly used anywhere, just emit a
// warning for now.
if self.tcx.sess.opts.debugging_opts.incremental.is_some() {
let msg = format!("Quasi-quoting might make incremental \
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this warning be rate-limited and spanned?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I could thread a span through for error reporting.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, nit: lower-case "quasi-quoting might...."

But span seems pretty useful

@michaelwoerister
Copy link
Member Author

Added a span to the warning.

@nikomatsakis
Copy link
Contributor

@michaelwoerister style nit: warnings begin with lower-case letter I think. other than, r=me

@michaelwoerister
Copy link
Member Author

@bors r=nikomatsakis

For things like this (changing the text of a warning message), the online-editing feature is really rather convenient.

@bors
Copy link
Contributor

bors commented Nov 17, 2016

📌 Commit 23730a9 has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented Nov 18, 2016

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

@michaelwoerister
Copy link
Member Author

@bors r=nikomatsakis

Rebased.

@bors
Copy link
Contributor

bors commented Nov 18, 2016

📌 Commit c722a1e has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented Nov 19, 2016

⌛ Testing commit c722a1e with merge 49d3fd3...

bors added a commit that referenced this pull request Nov 19, 2016
ICH: Handle MacroDef HIR instances.

As of recently, `hir::MacroDef` instances are exported in crate metadata, which means we also store their ICH when doing incremental compilation. Even though exported macro definitions should not (yet) interact with incremental compilation, the ICH is also used for the general purpose crate hash, where macros should be included.

This PR implements ICH computation for `MacroDef`. In theory, the ICH of these MacroDefs is less stable than that of other HIR items, since I opted to just call the compiler-generated `Hash::hash()` for `Token::Interpolated` variants. `Token::Interpolated` contains AST data structures and it would have been a lot of effort to expand ICH computation to the AST too. Since quasi-quoting is rarely used *and* it would only make a difference if incremental compilation was extended to macros, the simpler implementation seemed like a good idea.

This fixes the problem reported in #37756. The test still fails because of broken codegen-unit support though.

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

4 participants