Add needs-target-has-atomic directive
#133736
Merged
Conversation
This file contains 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
Before this commit, the test writer has to specify platforms and architectures by hand for targets that have differing atomic width support. `#[cfg(target_has_atomic)]` is not quite the same because (1) you may have to specify additional matchers manually which has to be maintained individually, and (2) the `#[cfg]` blocks does not communicate to compiletest that a test would be ignored for a given target. This commit implements a `//@ needs-target-has-atomic` directive which admits a comma-separated list of required atomic widths that the target must satisfy in order for the test to run. ``` //@ needs-target-has-atomic: 8, 16, ptr ``` See <rust-lang#87377>. Co-authored-by: kei519 <masaki.keigo.q00@kyoto-u.jp>
|
rustbot has assigned @Mark-Simulacrum. Use |
rustbot
added
A-compiletest
|
r? compiler (or bootstrap) |
rustbot
added
the
T-compiler
jieyouxu
commented
Dec 2, 2024
Comment on lines
-174
to
+181
| let (name, comment) = match ln.split_once([':', ' ']) { | ||
| Some((name, comment)) => (name, Some(comment)), | ||
| let (name, rest) = match ln.split_once([':', ' ']) { | ||
| Some((name, rest)) => (name, Some(rest)), | ||
| None => (ln, None), | ||
| }; | ||
|
|
||
| // FIXME(jieyouxu): tighten up this parsing to reject using both `:` and ` ` as means to | ||
| // delineate value. | ||
| if name == "needs-target-has-atomic" { |
There was a problem hiding this comment.
Remark: I didn't want to adjust how directives accept : for name-value form vs name-comment form in this PR, so this temporarily accepts //@ needs-target-has-atomic 8,16,ptr but I want to only accept the colon form in the future.
There was a problem hiding this comment.
Yeah, I noted this in the original PR I think. It's fine as long as we acknowledge wanting to fix it in the future.
jieyouxu
commented
Dec 2, 2024
Comment on lines
+489
to
+490
| /// Known widths of `target_has_atomic`. | ||
| pub const KNOWN_TARGET_HAS_ATOMIC_WIDTHS: &[&str] = &["8", "16", "32", "64", "128", "ptr"]; |
There was a problem hiding this comment.
Remark: this is yet another hard-coded list 😅
compiler-errors
approved these changes
Dec 2, 2024
|
@bors r+ rollup |
bors
added
S-waiting-on-bors
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Dec 2, 2024
…llaumeGomez Rollup of 13 pull requests Successful merges: - rust-lang#133603 (Eliminate magic numbers from expression precedence) - rust-lang#133715 (rustdoc-json: Include safety of `static`s) - rust-lang#133721 (rustdoc-json: Add test for `impl Trait for dyn Trait`) - rust-lang#133725 (Remove `//@ compare-output-lines-by-subset`) - rust-lang#133730 (Add pretty-printer parenthesis insertion test) - rust-lang#133736 (Add `needs-target-has-atomic` directive) - rust-lang#133739 (Re-add myself to rotation) - rust-lang#133743 (Fix docs for `<[T]>::as_array`.) - rust-lang#133744 (Fix typo README.md) - rust-lang#133745 (Remove static HashSet for default IDs list) - rust-lang#133749 (mir validator: don't store mir phase) - rust-lang#133751 (remove `Ty::is_copy_modulo_regions`) - rust-lang#133757 (`impl Default for EarlyDiagCtxt`) r? `@ghost` `@rustbot` modify labels: rollup
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Dec 2, 2024
Rollup merge of rust-lang#133736 - jieyouxu:needs-target-has-atomic, r=compiler-errors Add `needs-target-has-atomic` directive Before this PR, the test writer has to specify platforms and architectures by hand for targets that have differing atomic width support. `#[cfg(target_has_atomic="...")]` is not quite the same because (1) you may have to specify additional matchers manually which has to be maintained individually, and (2) the `#[cfg]` blocks does not communicate to compiletest that a test would be ignored for a given target. This PR implements a `//@ needs-target-has-atomic` directive which admits a comma-separated list of required atomic widths that the target must satisfy in order for the test to run. ``` //@ needs-target-has-atomic: 8, 16, ptr ``` See <rust-lang#87377>. This PR supersedes rust-lang#133095 and is co-authored by `@kei519,` because it was somewhat subtle, and it turned out easier to implement than to review. rustc-dev-guide docs PR: rust-lang/rustc-dev-guide#2154
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Before this PR, the test writer has to specify platforms and architectures by hand for targets that have differing atomic width support.
#[cfg(target_has_atomic="...")]is not quite the same because (1) you may have to specify additional matchers manually which has to be maintained individually, and (2) the#[cfg]blocks does not communicate to compiletest that a test would be ignored for a given target.This PR implements a
//@ needs-target-has-atomicdirective which admits a comma-separated list of required atomic widths that the target must satisfy in order for the test to run.See #87377.
This PR supersedes #133095 and is co-authored by @kei519, because it was somewhat subtle, and it turned out easier to implement than to review.
rustc-dev-guide docs PR: rust-lang/rustc-dev-guide#2154