represent slices with length in elements, not bytes #9885
Conversation
casting the `uint` to an `int` can result in printing high values as negative intege
This allows the indexing bounds check or other comparisons against an element length to avoid a multiplication by the size.
|
+1. This is especially important on ARM where there is no hardware integer division. |
| // except according to those terms. | ||
|
|
||
|
|
||
| // error-pattern:index out of bounds: the len is 2 but the index is -1 |
There was a problem hiding this comment.
Why was this test removed?
There was a problem hiding this comment.
It was testing that the index was printed wrong. I switched the index to being printed as a uint instead of an int, so the test no longer has a purpose. I added a new test for a very high index that was previously handled wrong to replace these.
GitHub thinks I moved small-negative-indexing.rs and edited it, but I really removed that too and wrote a new one.
|
It looks like the assumption about byte vs count is pretty sprawling throughout the codebase. When you were looking at these, did you notice an obvious trend which would make this pattern hard-coded in fewer places? I would expect that at least slice construction would be a pretty refactorable portion, but perhaps indexing may be as well. I'm also unfamiliar with the reasons for why we went with byte length as opposed to element count in the first place as well, and I'd want to understand why we did it before we change it. |
|
@alexcrichton: I don't know why byte count was ever used for length/capacity in vectors and length in slices. It results in a division or checked multiplication + extra branch being required for correct bounds checks, and a division to convert to the commonly needed element length. The only time we use length in bytes is the amortized cost of reallocation in |
|
I figured out the bug. It was caused by an array of zero-size types with destructors, because I replaced our manual byte offset derived from the non-zero length with a normal type-based offset. |
|
Since this only updates slices it introduces a division to borrowing. Do you plan to update unique vectors as well? |
|
Nice work! 🍬 |
The goal here is to avoid requiring a division or multiplication to compare against the length. The bounds check previously used an incorrect micro-optimization to replace the division by a multiplication, but now neither is necessary *for slices*. Unique/managed vectors will have to do a division to get the length until they are reworked/replaced.
| @@ -539,6 +538,40 @@ pub fn get_base_and_len(bcx: @mut Block, | |||
| } | |||
| } | |||
|
|
|||
| pub fn get_base_and_len(bcx: @mut Block, llval: ValueRef, vec_ty: ty::t) -> (ValueRef, ValueRef) { | |||
| //! | |||
There was a problem hiding this comment.
FWIW, this will mean the doc-string of get_base_and_len is ""; the rest of the block is ignored (it's treated as a normal comment).
There was a problem hiding this comment.
Yeah, I just copy-pasted it from above since I plan on removing the other function soon. I didn't notice it was missing the other ! markers.
Don't lint `string_lit_as_bytes` in match scrutinees fixes rust-lang#9885 changelog: `string_lit_as_bytes`: Don't lint in match scrutinees
The goal here is to avoid requiring a division or multiplication to compare against the length. The bounds check previously used an incorrect micro-optimization to replace the division by a multiplication, but now neither is necessary for slices. Unique/managed vectors will have to do a division to get the length until they are reworked/replaced.