Skip to content

Commit

Permalink
refactor: eliminate unnecessary index slicing
Browse files Browse the repository at this point in the history
This might have some slight performance improvements from
eliminating `match src[len..].bytes().next() {}`, but it's mostly
aimed at cutting out opportunities to panic.
  • Loading branch information
SKalt committed Jun 23, 2024
1 parent 7f46c8b commit 377e826
Show file tree
Hide file tree
Showing 8 changed files with 72 additions and 56 deletions.
10 changes: 4 additions & 6 deletions src/ambiguous/domain_or_tagged_ref.rs
Original file line number Diff line number Diff line change
Expand Up @@ -88,13 +88,12 @@ fn to_large_err(e: err::Error<u8>) -> err::Error<u16> {
impl<'src> DomainOrRefSpan<'src> {
pub(crate) fn new(src: &'src str) -> Result<Self, Error> {
let left = HostOrPathSpan::new(src, HostOrPathKind::Any)?;
let right_src = &src[left.len()..]; // TODO: consolidate with rest
let mut len = left.short_len().widen().upcast(); // current possible max: 255
let right = match right_src.bytes().next() {
let right = match src.as_bytes().get(len as usize) {
Some(b'/') | Some(b'@') | None => None,
Some(b':') => {
len = len.saturating_add(1); // +1 for the ':'
let right = PortOrTagSpan::new(&right_src[1..], Port).map_err(|e| {
let right = PortOrTagSpan::new(&src[len as usize..], Port).map_err(|e| {
Error::at(
len.saturating_add(e.index() as u16), // ok since len <= 256, so len + u8::MAX < u16::MAX
e.kind(),
Expand All @@ -111,8 +110,7 @@ impl<'src> DomainOrRefSpan<'src> {
};

len = len.saturating_add(right.map(|r| r.short_len().widen().upcast()).unwrap_or(0));
let rest = &src[len as usize..];
match rest.bytes().next() {
match src.as_bytes().get(len as usize) {
Some(b'@') | None => {
// since the next section must be a digest, the right side must be a tag
let path = PathSpan::try_from(left).map_err(to_large_err)?;
Expand Down Expand Up @@ -143,7 +141,7 @@ impl<'src> DomainOrRefSpan<'src> {
Path => {
// need to extend the path
let path = PathSpan::try_from(left)?
.extend(rest)
.extend(&src[len as usize..])
.map_err(to_large_err)?;

let tag = if let Some(t) = right {
Expand Down
1 change: 1 addition & 0 deletions src/digest/algorithm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ impl<'src> AlgorithmSpan<'src> {
if u8::from(len) >= max_len {
break;
} else {
// Note: using .get() seems to slow down performance here
match src.as_bytes()[u8::from(len) as usize] {
b':' => break,
b'+' | b'.' | b'_' | b'-' => {
Expand Down
18 changes: 8 additions & 10 deletions src/digest/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,28 +95,26 @@ pub(crate) struct DigestSpan<'src> {

impl<'src> DigestSpan<'src> {
pub(crate) fn new(src: &'src str) -> Result<Self, Error> {
let (algorithm, compliance) = AlgorithmSpan::new(src)?;
let mut len = algorithm.short_len().widen(); // max 255
let rest = &src[len.as_usize()..];
len = match rest.bytes().next() {
let (algorithm_span, compliance) = AlgorithmSpan::new(src)?;
let mut len = algorithm_span.short_len().widen(); // max 255

len = match src.as_bytes().get(len.as_usize()) {
Some(b':') => len.checked_add(1).ok_or(err::Kind::AlgorithmTooLong),
None => Err(err::Kind::AlgorithmMissing),
_ => Err(err::Kind::AlgorithmInvalidChar),
}
.map_err(|kind| Error::at(len.into(), kind))?;
let rest = &src[len.as_usize()..];
let (encoded, compliance) = EncodedSpan::new(rest, compliance)
let (encoded, compliance) = EncodedSpan::new(&src[len.as_usize()..], compliance)
.map_err(|e| Error::at(e.index().saturating_add(len.into()), e.kind()))?; // safe since len can be at most 256 and e.index() can be at most 1024

{
let rest = &src[len.as_usize()..];
let algorithm = Algorithm::from_span(src, algorithm);
let encoded = Encoded::from_span(rest, encoded);
let algorithm = Algorithm::from_span(src, algorithm_span);
let encoded = Encoded::from_span(&src[len.as_usize()..], encoded);
encoded.validate_algorithm(&algorithm, compliance)?;
}

Ok(Self {
algorithm,
algorithm: algorithm_span,
encoded,
compliance,
})
Expand Down
73 changes: 43 additions & 30 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@
#![warn(clippy::try_err)]
#![warn(clippy::todo)]
#![warn(clippy::redundant_clone)]
// #![warn(clippy::unreachable)] // used too often to enable
// #![warn(clippy::indexing_slicing)] // used too often to enable
// #![warn(clippy::unreachable)] // used too often to enable
// #![warn(clippy::or_fun_call)] // warns about ok_or(Error::at(...))
pub(crate) mod ambiguous;
pub mod digest;
Expand Down Expand Up @@ -75,19 +75,18 @@ impl<'src> RefSpan<'src> {
DomainOrRefSpan::TaggedRef(_) => None,
};
let mut index: u16 = domain.map(|d| d.short_len().upcast()).unwrap_or(0); // current max: 256
let rest = &src[prefix.len()..];
let path = match rest.bytes().next() {
let path = match src.as_bytes().get(prefix.len()) {
Some(b'/') => match prefix {
DomainOrRefSpan::TaggedRef((path_start, tag)) => match tag {
Some(_) => unreachable!(),
// ^^^^^^^^^^^^ if a tag is present and is followed
// by a `/`, it's PortInvalidChar error
None => path_start.extend(rest),
None => path_start.extend(&src[prefix.len()..]),
// e.g. "cant_be_host/more_path" needs to entirely match as path
},
DomainOrRefSpan::Domain(_) => {
index = index.saturating_add(1); // consume the leading slash; ok since index <= 256
let rest = &rest[1..];
let rest = &src[prefix.len() + 1..];
PathSpan::new(rest)
}
}
Expand All @@ -108,25 +107,29 @@ impl<'src> RefSpan<'src> {
if index > name::MAX_LEN.into() {
return Error::at(255, err::Kind::NameTooLong).into();
}
let rest = &src[index as usize..];
// let rest = &src[index as usize..];
let tag = match prefix {
DomainOrRefSpan::TaggedRef((_, right)) => match right {
Some(tag) => Ok(Some(tag)),
None => match rest.bytes().next() {
Some(b':') => TagSpan::new(&rest[1..]).map_err(|e| e.into()).map(Some),
None => match src.as_bytes().get(index as usize) {
Some(b':') => TagSpan::new(&src[index as usize + 1..])
.map_err(|e| e.into())
.map(Some),
Some(b'@') | None => Ok(None),
Some(_) => Error::at(0, err::Kind::PathInvalidChar).into(),
},
},
DomainOrRefSpan::Domain(_) => match rest.bytes().next() {
Some(b':') => TagSpan::new(&rest[1..]).map(Some).map_err(|e| {
Error::at(
index
DomainOrRefSpan::Domain(_) => match src.as_bytes().get(index as usize) {
Some(b':') => TagSpan::new(&src[index as usize + 1..])
.map(Some)
.map_err(|e| {
Error::at(
index
.saturating_add(1u16) // +1 to account for the leading ':'
.saturating_add(e.index().into()),
e.kind(),
)
}),
e.kind(),
)
}),
Some(_) | None => Ok(None),
},
}?;
Expand All @@ -137,8 +140,7 @@ impl<'src> RefSpan<'src> {
.map(|t: u16| t.saturating_add(1)) // +1 for the leading ':'
.unwrap_or(0_u16),
);
let rest = &src[index as usize..];
let digest = match rest.bytes().next() {
let digest = match src.as_bytes().get(index as usize) {
Some(b'@') => {
index = index.saturating_add(1); // max 385
DigestSpan::new(&src[index as usize..])
Expand Down Expand Up @@ -181,10 +183,6 @@ impl<'src> RefSpan<'src> {
.unwrap_or(0)
}

fn domain_range(&self) -> Range<usize> {
0..self.name.domain.map(|d| d.len()).unwrap_or(0)
}

fn port_range(&self) -> Option<Range<usize>> {
let domain = self.name.domain?;
let port = domain.port?;
Expand Down Expand Up @@ -256,7 +254,7 @@ impl<'src> ImgRef<'src> {
}
/// The port part of the domain, if present. This does not include the leading `:`.
pub fn port(&self) -> Option<&str> {
self.span.port_range().map(|r| &self.src[r])
self.span.port_range().and_then(|r| self.src.get(r))
}
fn path_str(&self) -> &str {
let range = self.span.path_range();
Expand All @@ -268,14 +266,19 @@ impl<'src> ImgRef<'src> {
}
/// Accessor the tag part of the reference NOT including the leading `:`
pub fn tag(&self) -> Option<&str> {
self.span.tag_range().map(|range| &self.src[range])
self.span.tag_range().and_then(|range| {
debug_assert!(self.src.get(range.clone()).is_some());
self.src.get(range)
})
}
/// Accessor for the optional digest part of the reference NOT including the leading `@`
pub fn digest(&self) -> Option<Digest<'src>> {
self.span.digest_range().and_then(|range| {
self.span
.digest
.map(|span| Digest::from_span(&self.src[range], span))
self.span.digest.and_then(|digest_span| {
debug_assert!(self.src.get(self.span.digest_index()..).is_some());
Some(Digest::from_span(
self.src.get(self.span.digest_index()..)?,
digest_span,
))
})
}
}
Expand Down Expand Up @@ -337,7 +340,6 @@ impl<'src> CanonicalSpan<'src> {
))?;
Ok(Self { span })
}
mirror_inner_method!(domain_range, Range<usize>);
mirror_inner_method!(path_range, Range<usize>);
mirror_inner_method!(name_range, Range<usize>);
mirror_inner_method!(tag_range, Option<Range<usize>>);
Expand Down Expand Up @@ -374,7 +376,15 @@ impl<'src> CanonicalImgRef<'src> {
Ok(Self { src, span })
}
fn domain_str(&self) -> &str {
&self.src[self.span.domain_range()]
self.span
.span
.name
.domain
.and_then(|d| {
debug_assert!(self.src.get(0..d.len()).is_some());
self.src.get(0..d.len())
})
.unwrap_or("")
}
fn path_str(&self) -> &str {
let path = &self.src[self.span.path_range()];
Expand Down Expand Up @@ -409,7 +419,10 @@ impl<'src> CanonicalImgRef<'src> {
/// The tag component of the canonical image reference, if present.
pub fn tag(&self) -> Option<&str> {
// tags aren't required for canonical refs
self.span.tag_range().map(|range| &self.src[range])
self.span.tag_range().and_then(|range| {
debug_assert!(self.src.get(range.clone()).is_some());
self.src.get(range)
})
}
#[allow(clippy::unwrap_used)]
/// The digest component of the canonical image reference.
Expand Down
6 changes: 3 additions & 3 deletions src/name/domain/ipv6.rs
Original file line number Diff line number Diff line change
Expand Up @@ -161,8 +161,8 @@ impl<'src> Ipv6Span<'src> {
.checked_add(1)
.ok_or(Error::at(u8::MAX, err::Kind::Ipv6TooLong))?;
}
debug_assert!(src.as_bytes()[0] == b'[');
debug_assert!(src.as_bytes()[index.as_usize()] == b']');
debug_assert!(src.as_bytes().first() == Some(&b'['));
debug_assert!(src.as_bytes().get(index.as_usize()) == Some(&b']'));
index = index
.checked_add(1) // consume t he closing bracket
.ok_or(Error::at(u8::MAX, err::Kind::Ipv6TooLong))?;
Expand All @@ -183,7 +183,7 @@ impl<'src> Ipv6Span<'src> {
#[cfg(test)]
mod test {
use crate::span::Lengthy;

#[allow(clippy::indexing_slicing)]
fn should_work(ip: &str) {
match super::Ipv6Span::new(ip) {
Ok(span) => assert_eq!(
Expand Down
12 changes: 10 additions & 2 deletions src/name/domain/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ impl<'src> DomainSpan<'src> {
pub(crate) fn new(src: &'src str) -> Result<Self, Error> {
let host = HostSpan::new(src)?;
let len: u16 = host.short_len().widen().into(); // max 255 chars
let port = match &src[host.len()..].bytes().next() {
let port = match src.as_bytes().get(host.len()) {
Some(b':') => PortSpan::new(&src[host.len() + 1..])
.map(Some)
.map_err(|e| Error::at(len.saturating_add(e.index().into()), e.kind())),
Expand Down Expand Up @@ -166,7 +166,15 @@ impl<'src> Domain<'src> {
pub fn port(&self) -> Option<&str> {
let port = self.span.port?;
let start = self.span.host.len() + 1; // +1 for the leading ':'
Some(&self.src[start..start + port.len()])
let result = self.src.get(start..start + port.len());
debug_assert!(
result.is_some(),
"{src:?}[{start}..{end}] is None",
src = self.src,
start = start,
end = start + port.len()
);
result
}
}

Expand Down
6 changes: 2 additions & 4 deletions src/name/path.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,15 +59,13 @@ impl<'src> PathSpan<'src> {
pub(crate) fn parse_from_slash(src: &'src str) -> Result<Option<Self>, Error> {
let mut index: u8 = 0;
loop {
let next = src[index as usize..].bytes().next();
index = match next {
index = match src.as_bytes().get(index as usize) {
Some(b'/') => index.checked_add(1).ok_or(err::Kind::PathTooLong),
None | Some(b':') | Some(b'@') => break,
Some(_) => Err(err::Kind::PathInvalidChar),
}
.map_err(|kind| Error::at(index, kind))?;
let rest = &src[index as usize..];
let component = Self::parse_component(rest).map_err(|e| {
let component = Self::parse_component(&src[index as usize..]).map_err(|e| {
let kind = match e.kind() {
err::Kind::PathMissing => err::Kind::PathComponentInvalidEnd,
kind => kind,
Expand Down
2 changes: 1 addition & 1 deletion src/span.rs
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ where
}
#[inline]
fn span_of(&self, src: &'src str) -> &'src str {
&src[..self.len()]
&src[..self.len()] // FIXME: use .get() to avoid panics
}
}

Expand Down

0 comments on commit 377e826

Please sign in to comment.