Skip to content

syntax: ABI-oblivious grammar - #65884

Merged
bors merged 6 commits into
rust-lang:masterfrom
Centril:non-hardcoded-abis
Nov 7, 2019
Merged

syntax: ABI-oblivious grammar#65884
bors merged 6 commits into
rust-lang:masterfrom
Centril:non-hardcoded-abis

Conversation

@Centril

@CentrilCentril commented Oct 28, 2019

Copy link
Copy Markdown
Contributor

This PR has the following effects:

  1. extern $lit is now legal where $lit:literal and $lit is substituted for a string literal.

  2. extern "abi_that_does_not_exist" is now syntactically legal whereas before, the set of ABI strings was hard-coded into the grammar of the language. With this PR, the set of ABIs are instead validated and translated during lowering. That seems more appropriate.

  3. ast::FloatTy is now distinct from rustc_target::abi::FloatTy. The former is used substantially more and the translation between them is only necessary in a single place.

  4. As a result of 2-3, libsyntax no longer depends on librustc_target, which should improve pipe-lining somewhat.

cc @rust-lang/lang -- the points 1-2 slightly change the definition of the language but in a way which seems consistent with our general principles (in particular wrt. the discussions of turning things into semantic errors). I expect this to be uncontroversial but it's worth letting y'all know. :)

r? @varkor

@CentrilCentril added T-lang Relevant to the language team relnotes Marks issues that should be documented in the release notes of the next release. labels Oct 28, 2019
@CentrilCentril added this to the 1.40 milestone Oct 28, 2019
@rust-highfive

This comment has been minimized.

Sign in to view
@rust-highfiverust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 28, 2019
Centril
Centril commented Oct 28, 2019
View reviewed changes
Comment threadsrc/libsyntax/feature_gate/check.rs Outdated

@CentrilCentrilOct 28, 2019

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.

(The purpose of doing this is so that if we add a new ABI but forget to feature gate it, an ICE will occur and it won't pass CI.)

@petrochenkov

petrochenkov commented Oct 28, 2019

Copy link
Copy Markdown
Contributor

Assigning myself as well, I had some plans in this area.
cc #60493

@petrochenkovpetrochenkov self-assigned this Oct 28, 2019
@rust-highfive

This comment has been minimized.

Sign in to view
Centril
Centril commented Oct 28, 2019
View reviewed changes
Comment threadsrc/libsyntax/parse/parser.rs Outdated
@Centril
Centrilforce-pushed the non-hardcoded-abis branch 2 times, most recently from 07e5ff6 to f8a588bCompareOctober 28, 2019 05:34
@eddyb

eddyb commented Oct 28, 2019

Copy link
Copy Markdown
Contributor

I think @oli-obk moved FloatTy around, perhaps now we can have Primitive::{F32, F64} again instead of rustc_target::abi::FloatTy existing.

eddyb
eddyb reviewed Oct 28, 2019
View reviewed changes
Comment threadsrc/libsyntax/ast.rs Outdated
@rust-highfive

This comment has been minimized.

Sign in to view
@nikomatsakis

nikomatsakis commented Oct 28, 2019

Copy link
Copy Markdown
Contributor

the points 1-2 slightly change the definition of the language but in a way which seems consistent with our general principles (in particular wrt. the discussions of turning things into semantic errors). I expect this to be uncontroversial but it's worth letting y'all know. :)

👍 from me

@Centril

Centril commented Oct 28, 2019

Copy link
Copy Markdown
ContributorAuthor

I think @oli-obk moved FloatTy around, perhaps now we can have Primitive::{F32, F64} again instead of rustc_target::abi::FloatTy existing.

@eddyb I don't have an opinion on this other than to say that it could be done in a follow-up. :)

@eddyb

eddyb commented Oct 28, 2019

Copy link
Copy Markdown
Contributor

it could be done in a follow-up. :)

My argument would be that it's confusing to have two copies of the same type.
But I was also hoping for @oli-obk's opinion on this.

petrochenkov
petrochenkov reviewed Oct 28, 2019
View reviewed changes
Comment threadsrc/libsyntax/ast.rs Outdated
Comment threadsrc/libsyntax_pos/symbol.rs Outdated

@petrochenkovpetrochenkovOct 28, 2019

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.

🦀

Comment threadsrc/libsyntax/parse/parser.rs Outdated
Comment threadsrc/libsyntax/parse/parser.rs Outdated
@CentrilCentril added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 28, 2019
@Centril
Centrilforce-pushed the non-hardcoded-abis branch 2 times, most recently from f66f4a1 to 9a46580CompareOctober 29, 2019 05:59
@CentrilCentril added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 29, 2019
@bors

This comment has been minimized.

Sign in to view
petrochenkov
petrochenkov reviewed Oct 29, 2019
View reviewed changes
Comment threadsrc/libsyntax/parse/parser.rs Outdated
@petrochenkov

petrochenkov commented Oct 29, 2019

Copy link
Copy Markdown
Contributor

r=me after addressing the remaining comments, unless varkor wants to review as well.

@petrochenkovpetrochenkov removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 29, 2019
@Centril

Centril commented Nov 6, 2019

Copy link
Copy Markdown
ContributorAuthor

@bors r=petrochenkov

@bors

bors commented Nov 6, 2019

Copy link
Copy Markdown
Collaborator

📌 Commit 09979759ba46574d39d8ab0c4dc14566b918e949 has been approved by petrochenkov

@borsbors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 6, 2019
@bors

This comment has been minimized.

Sign in to view
@borsbors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 7, 2019
@Centril

Centril commented Nov 7, 2019

Copy link
Copy Markdown
ContributorAuthor

@bors r=petrochenkov

@bors

bors commented Nov 7, 2019

Copy link
Copy Markdown
Collaborator

📌 Commit 55f76cd has been approved by petrochenkov

@borsbors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 7, 2019
Centril added a commit to Centril/rust that referenced this pull request Nov 7, 2019
@Centril
Rollup merge ofrust-lang#65884- Centril:non-hardcoded-abis, r=petro…
883fe10
…chenkov
syntax: ABI-oblivious grammar
This PR has the following effects:
1. `extern $lit` is now legal where `$lit:literal` and `$lit` is substituted for a string literal.
2. `extern "abi_that_does_not_exist"` is now *syntactically* legal whereas before, the set of ABI strings was hard-coded into the grammar of the language. With this PR, the set of ABIs are instead validated and translated during lowering. That seems more appropriate.
3. `ast::FloatTy` is now distinct from `rustc_target::abi::FloatTy`. The former is used substantially more and the translation between them is only necessary in a single place.
4. As a result of 2-3, libsyntax no longer depends on librustc_target, which should improve pipe-lining somewhat.
cc @rust-lang/lang -- the points 1-2 slightly change the definition of the language but in a way which seems consistent with our general principles (in particular wrt. the discussions of turning things into semantic errors). I expect this to be uncontroversial but it's worth letting y'all know. :)
r? @varkor
@CentrilCentril mentioned this pull request Nov 7, 2019
Merged
bors added a commit that referenced this pull request Nov 7, 2019
@bors
Auto merge of#66180- Centril:rollup-c1ji943, r=Centril
50f8aad
Rollup of 5 pull requests
Successful merges:
- #59789 (Revert two unapproved changes to rustc_typeck.)
- #65752 (Use structured suggestions for missing associated items)
- #65884 (syntax: ABI-oblivious grammar)
- #65974 (A scheme for more macro-matcher friendly pre-expansion gating)
- #66017 (Add future incompatibility lint for `array.into_iter()`)
Failed merges:
- #66056 (rustc_metadata: Some reorganization of the module structure)
r? @ghost
@bors
bors merged commit 55f76cd into rust-lang:masterNov 7, 2019
@Centril
Centril deleted the non-hardcoded-abis branch November 7, 2019 11:39
flip1995 added a commit to Manishearth/rust-clippy that referenced this pull request Nov 7, 2019
@flip1995
Rustup torust-lang/rust#65884
e917b01
@eddybeddyb mentioned this pull request Nov 7, 2019
Merged
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Nov 8, 2019
@JohnTitor
Rollup merge ofrust-lang#66190- eddyb:primflt, r=Centril
1969f41
rustc_target: inline abi::FloatTy into abi::Primitive.
This effectively undoes a small part of @oli-obk's rust-lang#50967, now that the rest of the compiler doesn't use the `FloatTy` definition from `rustc_target`, post-rust-lang#65884.
@CentrilCentril modified the milestones: 1.40, 1.41Nov 13, 2019
@petrochenkovpetrochenkov mentioned this pull request May 5, 2020
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@petrochenkovpetrochenkovpetrochenkov left review comments
+1 more reviewer
@eddybeddybeddyb left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@petrochenkovpetrochenkov

Labels

relnotesMarks issues that should be documented in the release notes of the next release.S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.T-langRelevant to the language team

Projects

None yet

Milestone

1.41

Development

Successfully merging this pull request may close these issues.

9 participants

@Centril@rust-highfive@petrochenkov@eddyb@nikomatsakis@bors@scottmcm@oli-obk@varkor