Use mutable references as per semantics #161
Conversation
|
In this PR we add a (commented out) test that ensures that the semantics we enforce indeed leads to a compile error if used incorrectly: fn test_features_mut_lifetime_enforce() {
with_layer("roads.geojson", |mut layer| {
for _ in layer.features() {
// the next line is a compile error as layer is mut borrowed above
let _ = layer.defn();
}
});
}Should we add trybuild as a dev-dependency so that we can express this as a regular test? The crate does add a few heavy deps (syn, rustc), but it is only a dev dependency and may be okay. I think it'll be useful in this crate as we try to ensure borrow semantics from an underlying library that doesn't have those. |
|
it would be fine for me |
|
This makes a lot of sense to me - I think it's good to have the compiler enforce "rust like" API usage. Taking it a step further, do you think it would be worthwhile for things like: To propagate some kind of lifetime: To clarify that |
I think it is already implicitly inferred. See |
+ add `trybuild` + add test to verify `Layer::features` mut-borrow. Refer georust#159
|
Oh you're absolutely right, sorry. |
|
|
||
| #[test] | ||
| fn test_features_aliasing_compile_fail() { | ||
| let t = trybuild::TestCases::new(); // A compilation test that should fail. |
There was a problem hiding this comment.
Cool to see trybuild!
I've never seen tests like this outside of the actual compiler dev, but it makes sense!
There was a problem hiding this comment.
It's a bit of a heavy dep., but seems useful for this crate as we try to impose rust semantics on ffi libs. I figure we could always comment it out, and remove the deps if CI takes too long.
src/vector/vector_tests/mod.rs
Outdated
| #[test] | ||
| fn test_features_aliasing_compile_fail() { | ||
| let t = trybuild::TestCases::new(); // A compilation test that should fail. | ||
| t.compile_fail("tests/ui/01-features-aliasing-errors.rs"); |
There was a problem hiding this comment.
why is the tests folder called "ui"?
There was a problem hiding this comment.
Not sure; I just followed the instructions from the the trybuild crate docs. Not having the ui/ folder caused some errors as (I think) cargo tried to run them as normal tests, and failed. I didn't try a different folder name though.
There was a problem hiding this comment.
It seems that any folder inside tests/ works; have renamed it to compile-tests
|
uh i broke it by merging the other PR -_- |
|
@jdroenner Not sure what the failure is, but I'll take a closer look next week and try to resolve it. |
make #161 compatible with main
CHANGES.mdif knowledge of this change could be valuable to users.This builds on #158 and also adds quite a few breaking changes.
closes #159
closes #150