Fix unused lifetime in ArchDetail (#199) and add CI feature combination tests (#200)#201
Conversation
#199) Add _Lifetime(PhantomData<&'a ()>) variant to ArchDetail enum and corresponding unreachable arm in operands() match to ensure the 'a lifetime parameter is always used.
Add feature-checks job to test: - all features disabled - all features enabled - each arch feature individually - random subsets of varying sizes Uses a Python script that dynamically discovers arch_* features from Cargo.toml instead of hardcoding feature names.
tmfink
left a comment
tmfink
left a comment
There was a problem hiding this comment.
This change looks really good--just some comments on the python script
| MANIFEST = REPO_ROOT / "capstone-rs" / "Cargo.toml" | ||
| CARGO = os.environ.get("CARGO", "cargo") | ||
|
|
||
| failed = [] |
There was a problem hiding this comment.
If you use a global (especially one you modify), then make it ALL CAPS so that it's clearly a global.
However, I suggest you avoid the mutable global and instead have cargo_check() return an error (or append to a failed argument) so it's more clear how everything is tracked. Sometimes we need a global, but I don't think we do here.
| failed = [] | ||
|
|
||
|
|
||
| def cargo_check(label, *args): |
There was a problem hiding this comment.
Instead of requiring cargo args, I think we should instead exposing "pythonic" args:
default_features=Falseall_features=Falsefeatures=None: list of features
| @@ -0,0 +1,95 @@ | |||
| #!/usr/bin/env python3 | |||
There was a problem hiding this comment.
For convenience, you can make this file executable (chmod +x)--git tracks these file permissions.
| failed = [] | ||
|
|
||
|
|
||
| def cargo_check(label, *args): |
There was a problem hiding this comment.
I suggest adding python type hints to make the script easier to understand. If you use an LSP like basedpyright, it helps catch warnings/errors. Linters like ruff can also help.
| cargo_check("all-features", "--all-features") | ||
|
|
||
| # 3. Each arch feature individually | ||
| base = "std,full" |
There was a problem hiding this comment.
let's also have tests for different combinations of std/full being enabled/disabled.
- Remove mutable global by having cargo_check return an error string or None - Switch from raw cargo args to Pythonic keyword arguments: default_features, all_features, features - Add Python type hints throughout - Make file executable (chmod +x) - Test all four std/full on/off combinations
2 participants
Fix #199: Add
_Lifetime(PhantomData<&'a ()>)variant toArchDetailenum to ensure the'alifetime parameter is always used, even when no arch features are enabled. Also add#[non_exhaustive]toArchDetailandArchOperandsince their variants depend on feature flags.Fix #200: Add
feature-checksCI job using a Python script that dynamically discoversarch_*features fromCargo.tomland tests: