Skip to content

RFC3239: Implement cfg(target) - Part 2 - #96913

Merged
bors merged 3 commits into
rust-lang:masterfrom
Urgau:rfc3239-part2
May 25, 2022
Merged

RFC3239: Implement cfg(target) - Part 2#96913
bors merged 3 commits into
rust-lang:masterfrom
Urgau:rfc3239-part2

Conversation

@Urgau

@UrgauUrgau commented May 10, 2022

Copy link
Copy Markdown
Member

This pull-request implements the compact cfg(target(..)) part of RFC 3239.

I recommend reviewing this PR on a per commit basics, because of some moving parts.

cc @GuillaumeGomez
r? @petrochenkov

@rustbotrustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label May 10, 2022
@rust-highfiverust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 10, 2022
GuillaumeGomez
GuillaumeGomez reviewed May 10, 2022
View reviewed changes
Comment threadcompiler/rustc_attr/src/builtin.rs Outdated
GuillaumeGomez
GuillaumeGomez reviewed May 10, 2022
View reviewed changes
Comment threadcompiler/rustc_attr/src/lib.rs Outdated
@GuillaumeGomezGuillaumeGomez mentioned this pull request May 10, 2022
Closed
petrochenkov
petrochenkov reviewed May 21, 2022
View reviewed changes
Comment threadsrc/test/ui/cfg/cfg-target-compact.rs Outdated
@petrochenkov

petrochenkov commented May 21, 2022

Copy link
Copy Markdown
Contributor

The implementation looks significantly overcomplicated due to all the multi-spans and supporting #96913 (comment).

I suggest to get rid of all of that, lower target(a, b = "x", c(y), "d") to all(target_a, target_b = "x", target_c(y), "d") immediately, and then continue processing it as any other predicate.

@petrochenkov

petrochenkov commented May 21, 2022

Copy link
Copy Markdown
Contributor

This part is not outright harmful like #96909, but I'd still be interested to see it dogfood-ed on the rust-lang/rust codebase to see how much useful it is, because I'm not sure whether it actually pulls its weight.

@petrochenkovpetrochenkov 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 May 21, 2022
@Urgau

Urgau commented May 23, 2022

Copy link
Copy Markdown
MemberAuthor

The implementation looks significantly overcomplicated due to all the multi-spans and supporting #96913 (comment).

As requested I have removed #96913 (comment) but I kept the multi-spans because without them the lint from the check cfg are confusing as they would just show the last part.

I suggest to get rid of all of that, lower target(a, b = "x", c(y), "d") to all(target_a, target_b = "x", target_c(y), "d") immediately, and then continue processing it as any other predicate.

Isn't this what is already done ? Or you mean doing this in the parser or somewhere else ?

This part is not outright harmful like #96909, but I'd still be interested to see it dogfood-ed on the rust-lang/rust codebase to see how much useful it is, because I'm not sure whether it actually pulls its weight.

I suppose this would be for another PR, not this one, right ?

@rustbot ready

@rustbotrustbot 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 May 23, 2022
@petrochenkov

petrochenkov commented May 23, 2022

Copy link
Copy Markdown
Contributor

I suppose this would be for another PR, not this one, right ?

Yes, once this change reaches bootstrap compiler.

@petrochenkov

petrochenkov commented May 23, 2022

Copy link
Copy Markdown
Contributor

Or you mean doing this in the parser or somewhere else ?

No, no, during cfg evaluation, roughly in the same place as now.

I see that right now errors like "cfg predicate key must be an identifier" are reported in two places.
I'd expect them to be reported once on the desugared all(target_a, target_b = "x", target_c(y), "d") representation.

I kept the multi-spans because without them the lint from the check cfg are confusing as they would just show the last part.

Could you perhaps leave this purely diagnostic change to a separate PR?
Right now the multi-span support changes represent a significant part of the PR while not being necessary for functional correctness.
(FWIW, highlighting only the last part would be fine by me.)

@petrochenkovpetrochenkov 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 May 23, 2022
@Urgau

Urgau commented May 24, 2022

Copy link
Copy Markdown
MemberAuthor

I kept the multi-spans because without them the lint from the check cfg are confusing as they would just show the last part.

Could you perhaps leave this purely diagnostic change to a separate PR?

Sure, done.

I'd expect them to be reported once on the desugared all(target_a, target_b = "x", target_c(y), "d") representation.

Done, but the way it's done feels like a hack to me which is why I've put it in the last commit. This approach would also completely remove the possibility of having multi-spans or simply better diagnostics. Anyway If you prefer this approach I will squash the commits.

@rustbot ready

@rustbotrustbot 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 May 24, 2022
petrochenkov
petrochenkov reviewed May 24, 2022
View reviewed changes
Comment threadcompiler/rustc_feature/src/active.rs Outdated
Comment threadcompiler/rustc_attr/src/builtin.rs Outdated
Comment threadcompiler/rustc_attr/src/builtin.rs Outdated
Comment threadcompiler/rustc_trait_selection/src/traits/on_unimplemented.rs Outdated
Comment threadcompiler/rustc_attr/src/builtin.rs Outdated
Comment threadcompiler/rustc_attr/src/builtin.rs Outdated
Comment threadcompiler/rustc_attr/src/builtin.rs Outdated
@petrochenkov

petrochenkov commented May 24, 2022

Copy link
Copy Markdown
Contributor

Looks much better now.

@petrochenkovpetrochenkov 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 May 24, 2022
@Urgau

Urgau commented May 24, 2022

Copy link
Copy Markdown
MemberAuthor

The implementation is now strangely clean and simple. 😄

@rustbot ready

@rustbotrustbot 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 May 24, 2022
@petrochenkov

petrochenkov commented May 24, 2022

Copy link
Copy Markdown
Contributor

@bors r+

@bors

bors commented May 24, 2022

Copy link
Copy Markdown
Collaborator

📌 Commit b9ae3db 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-review Status: Awaiting review from the assignee but also interested parties. labels May 24, 2022
@Dylan-DPCDylan-DPC mentioned this pull request May 25, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request May 25, 2022
@bors
Auto merge ofrust-lang#97388- Dylan-DPC:rollup-tfuc4tf, r=Dylan-DPC
fe9c64d
Rollup of 5 pull requests
Successful merges:
- rust-lang#95953 (Modify MIR building to drop repeat expressions with length zero)
- rust-lang#96913 (RFC3239: Implement `cfg(target)` - Part 2)
- rust-lang#97233 ([RFC 2011] Library code)
- rust-lang#97370 (Minor improvement on else-no-if diagnostic)
- rust-lang#97384 (Fix metadata stats.)
Failed merges:
r? `@ghost`
`@rustbot` modify labels: rollup
@bors
bors merged commit c12a36a into rust-lang:masterMay 25, 2022
@rustbotrustbot added this to the 1.63.0 milestone May 25, 2022
@Urgau
Urgau deleted the rfc3239-part2 branch May 5, 2023 16:46
@UrgauUrgau mentioned this pull request Sep 24, 2024
Open
3 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@GuillaumeGomezGuillaumeGomezGuillaumeGomez left review comments
@petrochenkovpetrochenkovpetrochenkov left review comments

Assignees

@petrochenkovpetrochenkov

Labels

S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.T-compilerRelevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

1.63.0

Development

Successfully merging this pull request may close these issues.

6 participants

@Urgau@petrochenkov@bors@GuillaumeGomez@rust-highfive@rustbot