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

s/compiler-flags/compile-flags in compiletest #94243

Merged
merged 2 commits into from
Apr 11, 2022

Conversation

compiler-errors
Copy link
Member

Also make compiletest panic so this doesn't happen in the future! I literally always forget which it's called, so I wanted to make my life easier in the future.

Also open to the possibility of parsing both.

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Feb 22, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 22, 2022
@Mark-Simulacrum
Copy link
Member

Hm, I definitely think the validation is a good thing. I'm wondering if we should get stricter about what's present in test headers that looks like it might be a directive (but didn't get parsed as one) -- it's probably pretty rare that unintentionally users write things that look akin to directives?

Would you be up for adjusting compiletest to lint against unparsed name/value directives, at least? Those seem like the easiest to catch.

Eventually we might want to consider moving them to some kind of Rust-like #![compiletest::foo = ...] syntax which is probably not too hard to parse in compiletest and is much easier to lint on. cc #87396 on that thought, I guess.

@compiler-errors
Copy link
Member Author

@Mark-Simulacrum, sure, denying // name: value you mean? If so, can do

@Mark-Simulacrum
Copy link
Member

Yeah, essentially -- my hope is that there's not too many false positives (maybe we can ignore some specific names to avoid common false positives if needed).

@compiler-errors
Copy link
Member Author

I can try this.

@rustbot author

@rustbot rustbot 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 Feb 22, 2022
@JohnCSimon
Copy link
Member

@compiler-errors
Ping from triage: Can you please post the status of this PR?
Thank you.

@compiler-errors
Copy link
Member Author

Ah, yeah, have kinda ignored this. I will work on it this week!

@compiler-errors
Copy link
Member Author

Hm, probably won't get around to fixing this in the general case. @Mark-Simulacrum, can we merge this as-is? Otherwise I'll just close this.

@rustbot ready

@rustbot rustbot 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 Apr 10, 2022
@Mark-Simulacrum
Copy link
Member

I'm fine with merging as-is.

@bors r+

@bors
Copy link
Contributor

bors commented Apr 11, 2022

📌 Commit 52dd0b6 has been approved by Mark-Simulacrum

@bors bors 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 Apr 11, 2022
@bors
Copy link
Contributor

bors commented Apr 11, 2022

⌛ Testing commit 52dd0b6 with merge 24f73cee380c44349067b397bb0b2c74508f71b4...

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Apr 11, 2022
…, r=Mark-Simulacrum

`s/compiler-flags/compile-flags` in compiletest

Also make compiletest panic so this doesn't happen in the future! I literally always forget which it's called, so I wanted to make my life easier in the future.

Also open to the possibility of parsing both.
@Dylan-DPC
Copy link
Member

@bors retry (included in rollup)

@bors
Copy link
Contributor

bors commented Apr 11, 2022

⌛ Testing commit 52dd0b6 with merge d12b857...

@bors
Copy link
Contributor

bors commented Apr 11, 2022

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing d12b857 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 11, 2022
@bors bors merged commit d12b857 into rust-lang:master Apr 11, 2022
@rustbot rustbot added this to the 1.62.0 milestone Apr 11, 2022
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@rust-timer
Copy link
Collaborator

This benchmark run did not return any relevant results.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@compiler-errors compiler-errors deleted the compiler-flags-typo branch August 11, 2023 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants