Merge compiletest directives to do with llvm versions - #160637
Merge compiletest directives to do with llvm versions#160637WaffleLapkin wants to merge 5 commits into
Conversation
|
Some changes occurred in src/tools/compiletest cc @jieyouxu Some changes occurred in coverage tests. cc @Zalathar
|
rustbot
added
A-compiletest
|
|
rustbot
added
F-explicit_tail_calls
| assert_eq!(extract_llvm_version("9.0.1-rust-1.43.0-dev"), Version::new(9, 0, 1)); | ||
| assert_eq!(extract_llvm_version("9.3.1-rust-1.43.0-dev"), Version::new(9, 3, 1)); | ||
| assert_eq!(extract_llvm_version("10.0.0-rust"), Version::new(10, 0, 0)); | ||
| // assert_eq!(extract_llvm_version("4.5.6git"), Version::new(4, 5, 6)); |
There was a problem hiding this comment.
Is this an actual version string that llvm outputs? In that case I'll have to (partially?) revert the change to extract_llvm_version...
There was a problem hiding this comment.
I don't think so, this was probably just trying to hedge against trailing random things
There was a problem hiding this comment.
Unfortunately I take that back, I believe for certain LLVM builds, the version string can actually be NNN.N.Ngit, e.g. llvm/llvm-project#71742, in particular if the LLVM build isn't from rc/release tags?
This comment has been minimized.
This comment has been minimized.
Instead of having separate directives for major versions and whatnot, just use the semver version requirement.
…vm-version` If we don't want them to combine we should explicitly disallow them...
cfd0ca7 to
968a931
Compare
| - `llvm-version: cmp` — ignore if the LLVM version doesn't match the comparator; examples of comparators that can be used: | ||
| - `=22` — ignored if the LLVM major version does not match the given value | ||
| - `>=22` — ignored if the LLVM major version is less than the given value | ||
| - `<=22` — ignored if the LLVM major version is higher than the given major version | ||
| - `>=22, <=23` — ignored if the LLVM major version is outside the given range | ||
| - all of the above can specify minor and patch version too, `=22.1`, `<=22.1`, `>=22.1` | ||
| - for a full list see [`semver` crate documentation](https://docs.rs/semver/latest/semver/struct.Comparator.html) | ||
| - `system-llvm-version: cmp` — ignored if using a system LLVM and its version does not match the comparator; see above for examples of comparators | ||
| - `ignore-llvm-version: cmp` — ignored if LLVM version **matches** the comparator; see above for examples of comparators |
There was a problem hiding this comment.
Discussion (not necessarily for this PR, but): hm, right now, //@ edition uses the range literal format (e.g. 2015..2021. I think > et al. are entirely reasonable for //@ llvm-version or //@ system-llvm-version, just that edition would be a different syntax.
I almost wonder if we should just, change //@ edition to use >/</<=/>=? For edition, at the time it was added, there was just a need for a syntax, not necessarily married to ../..=.
| assert_eq!(extract_llvm_version("9.0.1-rust-1.43.0-dev"), Version::new(9, 0, 1)); | ||
| assert_eq!(extract_llvm_version("9.3.1-rust-1.43.0-dev"), Version::new(9, 3, 1)); | ||
| assert_eq!(extract_llvm_version("10.0.0-rust"), Version::new(10, 0, 0)); | ||
| // assert_eq!(extract_llvm_version("4.5.6git"), Version::new(4, 5, 6)); |
There was a problem hiding this comment.
Unfortunately I take that back, I believe for certain LLVM builds, the version string can actually be NNN.N.Ngit, e.g. llvm/llvm-project#71742, in particular if the LLVM build isn't from rc/release tags?
| let config: Config = cfg().llvm_version("10.6.2").build(); | ||
| assert!(!check_ignore(&config, "//@ exact-llvm-major-version: 10")); |
There was a problem hiding this comment.
Question: wait, does this test case actually still pass...?
|
(The overall change def makes sense to me though) |
rustbot
added
S-waiting-on-author
4 participants
I got confused by
min-llvm-versionandmax-llvm-major-versionbeing different not only inmin/max(-major).... So I decided to refactor everything to just use semver version requirements (
//@ llvm-version: <=22, etc). IMO this is much nicer.r? @jieyouxu