split_at audit
#5970
split_at audit
#5970
Conversation
robertbastian
requested review from
Manishearth,
sffc,
zbraniecki,
hsivonen,
echeran,
nciric and
a team
as code owners
robertbastian
requested review from
hsivonen
and removed request for
zbraniecki,
hsivonen,
echeran and
nciric
| let Some((&[lead, trail], remainder)) = self | ||
| .store | ||
| .split_at_checked(2) | ||
| .map(|(a, b)| (a.as_bytes(), b)) | ||
| else { | ||
| return Err(MultiNamedPlaceholderError::InvalidStore); | ||
| }; |
There was a problem hiding this comment.
| let Some((&[lead, trail], remainder)) = self | |
| .store | |
| .split_at_checked(2) | |
| .map(|(a, b)| (a.as_bytes(), b)) | |
| else { | |
| return Err(MultiNamedPlaceholderError::InvalidStore); | |
| }; | |
| let (&[lead, trail], remainder) = self | |
| .store | |
| .split_at_checked(2) | |
| .ok_or(MultiNamedPlaceholderError::InvalidStore) | |
| .map(|(a, b)| (a.as_bytes(), b))?; |
| let Some((placeholder_name, remainder)) = | ||
| remainder.split_at_checked(placeholder_len as usize) | ||
| else { | ||
| return Err(MultiNamedPlaceholderError::InvalidStore); | ||
| }; |
There was a problem hiding this comment.
| let Some((placeholder_name, remainder)) = | |
| remainder.split_at_checked(placeholder_len as usize) | |
| else { | |
| return Err(MultiNamedPlaceholderError::InvalidStore); | |
| }; | |
| let (placeholder_name, remainder) = | |
| remainder.split_at_checked(placeholder_len as usize) | |
| .ok_or(MultiNamedPlaceholderError::InvalidStore)?; |
| // SAFETY: cmp_len is at most self.code_units.len() | ||
| let (this, remainder) = unsafe { self.code_units.split_at_unchecked(cmp_len) }; |
There was a problem hiding this comment.
No need for unsafe here
| // SAFETY: cmp_len is at most self.code_units.len() | |
| let (this, remainder) = unsafe { self.code_units.split_at_unchecked(cmp_len) }; | |
| let (this, remainder) = self.code_units.split_at_checked(other.len()) | |
| .unwrap_or((self.code_units, &[])); |
There was a problem hiding this comment.
This is a very hot code path. split_at_checked introduces a bounds check that might not be optimized out.
There was a problem hiding this comment.
We got by with the bounds check so far and had no apparent performance issues. I don't think we should introduce unsafe unless we have actual numbers on what this affects, and whether optimizations are indeed inhibited.
There was a problem hiding this comment.
My code snippet also makes cmp_len unused, which removes the cmp::min on the previous line, but GitHub wouldn't let me make a suggestion spanning a deleted line.
Co-authored-by: Shane F. Carr <[email protected]>
robertbastian
dismissed stale reviews from Manishearth and nciric
via
72c37f0
This reverts commit 72c37f0.
| // SAFETY: cmp_len is at most self.code_units.len() | ||
| let (this, remainder) = unsafe { self.code_units.split_at_unchecked(cmp_len) }; |
There was a problem hiding this comment.
My code snippet also makes cmp_len unused, which removes the cmp::min on the previous line, but GitHub wouldn't let me make a suggestion spanning a deleted line.
|
Why did you unaccept the suggestions from code review? I wrote them with the intention to illustrate what I was talking about, but I expected that they would probably need some coding and formatting to make them actually compile. Note: "requested changes" is because I am against adding new unsafe code. The other changes are cosmetic but reduce the number of lines of code. |
With the 1.80 MSRV we can use the non-panicky alternatives now.