Bit testing can use unsafeRead because the length was verified #24
Conversation
| @@ -44,6 +44,5 @@ testBit (BS v sz) bitIx | |||
| | otherwise = do | |||
| let wordIx = bitIx `div` bitsPerWord | |||
| bitPos = bitIx `mod` bitsPerWord | |||
| w <- V.read v wordIx | |||
| w <- V.unsafeRead v wordIx | |||
There was a problem hiding this comment.
I think we would also need to check that bitIx >= 0 for this to be safe-ish. If you pass in a negative index, this could be very bad. Perhaps the bug is that the API takes an index using a signed type.
There was a problem hiding this comment.
In general, I am saddened that so much of the prelude and containers uses Int instead of Natural. Any index, the length, the initial size, etc. should all be Natural but are all Ints. I've used Natural in other places, but it does add some noise because there's a need to convert between Natural and Int in [lots of places to impedance match with the stuff that is written to use Int.
I think this is actually safe in practice because this is an internal module that is not exported to users, and it's only used from Algorithms.DFS as the latter iterates through graph elements (practically speaking, even the bigIx >= size should be unnecessary based on the usage).
I considered switching this interface to Nat, but it's using the vertexId, so either DFS is calling toEnum on all these so that BitSet can almost immediately call fromEnum to invoke the fromEnum to call the Vector operations, or the Vertex type changes to a Natural (and although it's supposed to be internal, it's visible and a much bigger change, and it still needs the fromEnum to call the Vector operations).
In fact, I switched the other read in this file to unsafe, then I benchmarked, the original safe, the unsafe in this PR, and a unsafe Nat version with the fromEnum/toEnum I discussed above. I'll email the criterion HTML separately. tl;dr Nat+unsafe is 33% slower than unsafe, and safe is 25% slower than unsafe.
There was a problem hiding this comment.
I had forgotten that this was an internal-only module.
In that case, what if we just renamed this to unsafeSetBit and unsafeTestBit and even removed the bounds check?
I was going to say that the only uses do their own checking, but that is not entirely true. If you mix vertices from another graph, you could hit an error (and we don't have any type-level enforcement of that). It looks like it would be safe if we just filtered the root [Vertex] inputs in the DFS module.
No description provided.