add tests with discontiguous owned data #444
Conversation
tests/array.rs
Outdated
|
|
||
| struct InsertOnDrop<T: Ord>(Rc<RefCell<BTreeSet<T>>>, Option<T>); | ||
| impl<T: Ord> InsertOnDrop<T> { | ||
| fn new(set: Rc<RefCell<BTreeSet<T>>>, value: T) -> Self { |
There was a problem hiding this comment.
Will you please remove this new method and just construct InsertOnDrop manually with InsertOnDrop(set.clone(), Some(x))? (new is used in only one place, and it would be nice to make the test more compact. I see the advantage of hiding the implementation detail that value should start as Some, but since the use is so close to the implementation, I prefer conciseness over encapsulation in this case.)
There was a problem hiding this comment.
No problem, that was an artefact of copypasta from another project where it was used more heavily
tests/array.rs
Outdated
| // discontiguous and non-zero offset | ||
| a.slice_inplace(s![.., 1..]); | ||
| } | ||
| // each item was dropped |
There was a problem hiding this comment.
Will you please change this comment to say "each item was dropped exactly once"?
tests/array.rs
Outdated
| assert_eq!(c.strides(), &[3, 2]); | ||
| assert_eq!(co.strides(), &[2, 1]); | ||
| assert_eq!(c.shape(), co.shape()); | ||
| itertools::assert_equal(c.iter(), co.iter()); |
There was a problem hiding this comment.
I'd prefer just assert_eq!(c, co) instead of itertools::assert_equal(), unless there's a specific reason for using itertools::assert_equal() here.
There was a problem hiding this comment.
No reason, I think I was just confused and thought PartialEq would check strides for some reason
|
Everything looks good to me. If you squash the PR into a single commit and force-push it to your branch ( (I wouldn't mind squashing it myself, but if I did that, then GitHub wouldn't let me mark the PR as merged, and you wouldn't be listed as the committer.) |
I think Github has an option you can pick from a dropdown near the "Merge" button that makes it do a squash? (unless that's what you're referring to, though I would have hoped/assumed that GitHub would have it work in such a way as to preserve this metadata) Edit: well, hm. It's not really clear from their documentation whether it works that way. I'll just squash and force push as you asked. |
|
Well, the "squash and merge" option is disabled for this repo, but even if it wasn't AFAICT GitHub doesn't let you do a "squash followed by --no-ff merge"; the "squash and merge" only lets you "squash followed by --ff-only merge" which loses the merge commit. I'd really like a button to "mark as merged" regardless of how I actually merged it (so I could freely squash stuff myself), but I don't see anything like that in the GitHub UI. I don't care all that much about the metadata, but I do like to give contributors credit for their contributions. Thanks for the PR! |
|
On further investigation, it looks like if "Allow edits from maintainers" is checked on the PR, then it might be possible for me to squash and force-push directly to the contributor's PR branch. I'll have to try that the next time I encounter this situation so that I don't have to bother the contributor with squashing. |
Sprinkle about a couple of tests that produce discontiguous owned arrays, including one RcArray. I figured there's no need for the tests to be that intensive, because ndarray handles ownership entirely orthogonally to slicing/strides; but anything is better than nothing.
This also adds a smoke test for
Dropdata. Given that ndarray never manually implsDropand that all usages offorgetand unsafestd::ptrmethods easily fit in a screenful of grep output[^1], there's little reason to believe this test should fail; but it does give bragging rights.[^1] I'll note there was one call to
ptr::writeinfrom_shape_fnthat made me give a double take before finally concluding that it was panic safe. Maybe that is what deserves to be tested instead...