tidy struggles to reason about revisions
Open
Nobody has claimed this yet.
A-compiletest
A-tidy
C-bug
T-bootstrap
- Dominant language
- Rust
- Stars
- 119k
- Forks
- 16.1k
- PR merge metrics
- PR metrics pending
Description
after the following diff:
tests/assembly: cross-compile x86-return-float
We choose to test for Linux and Windows instead of random other targets.
diff --git a/tests/assembly/x86-return-float.rs b/tests/assembly/x86-return-float.rs
index 423263c9673..35b9e2d18d3 100644
--- a/tests/assembly/x86-return-float.rs
+++ b/tests/assembly/x86-return-float.rs
@@ -1,19 +1,27 @@
//@ assembly-output: emit-asm
-//@ only-x86
// FIXME(#114479): LLVM miscompiles loading and storing `f32` and `f64` when SSE is disabled.
// There's no compiletest directive to ignore a test on i586 only, so just always explicitly enable
// SSE2.
// Use the same target CPU as `i686` so that LLVM orders the instructions in the same order.
//@ compile-flags: -Ctarget-feature=+sse2 -Ctarget-cpu=pentium4
// Force frame pointers to make ASM more consistent between targets
-//@ compile-flags: -Copt-level=3 -C force-frame-pointers
+//@ compile-flags: -C force-frame-pointers
+// At opt-level=3, LLVM can merge two movss into one movsd, and we aren't testing for that.
+//@ compile-flags: -Copt-level=2
//@ filecheck-flags: --implicit-check-not fld --implicit-check-not fst
-//@ revisions: normal win
-//@[normal] ignore-windows
-//@[win] only-windows
+//@ revisions: linux win
+//@ add-core-stubs
+//@ needs-llvm-components: x86
+//@[linux] compile-flags: --target i686-unknown-linux-gnu
+//@[win] compile-flags: --target i686-pc-windows-msvc
#![crate_type = "lib"]
#![feature(f16, f128)]
+#![feature(no_core)]
+#![no_core]
+
+extern crate minicore;
+use minicore::*;
I got this feedback from tidy:
fmt: checked 5834 files
tidy check
/checkout/tests/assembly/x86-return-float.rs: revision [unspecified] should not specify `needs-llvm-components:` as it doesn't need `--target`
/checkout/tests/assembly/x86-return-float.rs: revision linux should specify `needs-llvm-components:` as it has `--target` set
/checkout/tests/assembly/x86-return-float.rs: revision win should specify `needs-llvm-components:` as it has `--target` set
some tidy checks failed
I think this review should get pushback, but I'm not sure how best to phrase it.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with tests/assembly/x86-return-float.rs and reproduce the tidy check output for its linux and win revisions. Read how tidy interprets revisions, --target, and needs-llvm-components; done means the validation matches the intended per-revision directives or the issue has a precise, supported correction.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- testing-qa, tooling
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 38/100