RFC3239: Implement cfg(target) - Part 2 by Urgau · Pull Request #96913 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation22 Commits3 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
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
Relevant to the compiler team, which will review and decide on the PR/issue.
label
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.
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.
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")toall(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
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
I suppose this would be for another PR, not this one, right ?
Yes, once this change reaches bootstrap compiler.
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.)
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
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
The implementation is now strangely clean and simple. 😄
@rustbot ready
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
📌 Commit b9ae3db has been approved by petrochenkov
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
bors added a commit to rust-lang-ci/rust that referenced this pull request
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
3 tasks
Labels
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Relevant to the compiler team, which will review and decide on the PR/issue.