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

Cleanup handling of hygiene for built-in macros #64469

Merged
merged 4 commits into from Sep 15, 2019

Conversation

@matthewjasper
Copy link
Contributor

commented Sep 14, 2019

This makes most identifiers generated by built-in macros use def-site hygiene, not only the ones that previously used gensyms.

  • ExtCtxt::ident_of now takes a Span and is preferred to Ident::{from_str, from_str_and_span}
  • Remove Span::with_legacy_ctxt
    • assert now uses call-site hygiene because it needs to resolve panic unhygienically.
    • concat_idents now uses call-site hygiene because it wouldn't be very useful with def-site hygiene.
    • everything else is moved to def-site hygiene

r? @petrochenkov

src/librustc_lint/unused.rs Outdated Show resolved Hide resolved
@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented Sep 15, 2019

r=me with Centril's comment addressed

@matthewjasper matthewjasper force-pushed the matthewjasper:increase-hygiene-use branch from 1742e1e to 8ab67c8 Sep 15, 2019
@matthewjasper

This comment has been minimized.

Copy link
Contributor Author

commented Sep 15, 2019

@bors r=petrochenkov

@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 15, 2019

📌 Commit 8ab67c8 has been approved by petrochenkov

@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 15, 2019

⌛️ Testing commit 8ab67c8 with merge 117cdf3...

bors added a commit that referenced this pull request Sep 15, 2019
…nkov

Cleanup handling of hygiene for built-in macros

This makes most identifiers generated by built-in macros use def-site hygiene, not only the ones that previously used gensyms.

* `ExtCtxt::ident_of` now takes a `Span` and is preferred to `Ident::{from_str, from_str_and_span}`
* Remove `Span::with_legacy_ctxt`
    * `assert` now uses call-site hygiene because it needs to resolve `panic` unhygienically.
    * `concat_idents` now uses call-site hygiene because it wouldn't be very useful with def-site hygiene.
    * everything else is moved to def-site hygiene

r? @petrochenkov
let sp = cx.with_legacy_ctxt(sp);
// `core::panic` and `std::panic` are different macros, so we use call-site
// context to pick up whichever is currently in scope.
let sp = cx.with_call_site_ctxt(sp);

This comment has been minimized.

Copy link
@petrochenkov

petrochenkov Sep 15, 2019

Contributor

I'd rather use the call-site span in a more targeted way though, for the panic identifier specifically, and def-site for everything else.
(Not sure if this makes observable difference in this case, but it's a safer default.)

@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 15, 2019

☀️ Test successful - checks-azure
Approved by: petrochenkov
Pushing 117cdf3 to master...

@bors bors added the merged-by-bors label Sep 15, 2019
@bors bors merged commit 8ab67c8 into rust-lang:master Sep 15, 2019
5 checks passed
5 checks passed
homu Test successful
Details
pr Build #20190915.9 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
pr (LinuxTools) LinuxTools succeeded
Details
@matthewjasper matthewjasper deleted the matthewjasper:increase-hygiene-use branch Sep 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.