Use features instead of build.rs to select llvm_x_or_greater #9
Conversation
There was a problem hiding this comment.
This looks great! Just a few small nits noted in my review. I hope it will be clear to crate users that they need not (and should not) manually activate these new features - but I think that will probably be clear enough, especially if we add a comment in Cargo.toml to that effect?
build.rs
Outdated
| @@ -12,30 +12,9 @@ fn main() { | |||
| if cfg!(feature = "llvm-11") { | |||
| versions.push(11); | |||
| } | |||
| let selected_version = match versions.len() { | |||
| match versions.len() { | |||
| 0 => panic!("llvm-ir: Please select an LLVM version using a Cargo feature."), | |||
| 1 => versions[0], | |||
There was a problem hiding this comment.
tiny note, but since the let selected_version = was removed, we can also make this line be 1 => (), or similar.
Cargo.toml
Outdated
| llvm-11 = ["llvm-sys-110", "llvm-11-or-lower", "llvm-11-or-greater"] | ||
|
|
||
| # For convenience we set a number of configuration options to avoid | ||
| # checking complex combinations of features all the time. |
There was a problem hiding this comment.
Can we also add a comment here to the effect of "These features are not meant to be manually enabled; use the features above instead"?
There was a problem hiding this comment.
Also, referring to "configuration options" might be confusing now since they're no longer cargo:rustc-cfg. Maybe "For convenience, these automatically-enabled features allow us to avoid checking complex combinations of features all the time"?
Cargo.toml
Outdated
|
|
||
| # For convenience we set a number of configuration options to avoid | ||
| # checking complex combinations of features all the time. | ||
| llvm-8-or-greater = [] # At least version 9 |
There was a problem hiding this comment.
I'm confused by the comment "At least version 9". I'm fine with removing this comment (also on the next line)?
Cargo.toml
Outdated
| llvm-9 = ["llvm-sys-90"] | ||
| llvm-10 = ["llvm-sys-100"] | ||
| llvm-11 = ["llvm-sys-110"] | ||
| llvm-8 = ["llvm-sys-80", "llvm-8-or-lower"] |
There was a problem hiding this comment.
For symmetry we can have this also depend on llvm-8-or-greater
TheZoq2
force-pushed
the
fix_rust_analyzer
branch
from
5161657
to
7f62ecf
Compare
|
Thanks for the quick reply! I pushed an update with the comments fixed |
|
Awesome! Will merge this, thanks! |
I had a problem with rust-analyzer thinking the
debuglocfields were not present in most strutcs even though I'm building with llvm-11. Seing that those fields depend on CFG flags set in build.rs made mes suspect that this was the cause.To fix it, I added some extra feature flags to Cargo.toml which do the same thing. It's a bit ugly, but the inheritance of features at least makes the amount of needed features linear in the amount of llvm versions supported.
Sidenote: after making this change, I found a r-a issue claiming to fix this kind of problem, but it seems like it's not fully fixed rust-lang/rust-analyzer#4296. Especially as if I removed the debugloc-fields, it would tell me that the field is needed. We may want to report this to them