D6773: rust-hgpath: add HgPath and HgPathBuf structs to encapsulate handling of paths

[yuja (Yuya Nishihara)]

yuja added a comment.


  The series looks good as a start. Queued, thanks!
  
  > +impl ToString for HgPathError {
  
  Nit: `std::fmt::Display` can be implemented instead, and the `HgPathError`
  can be a `std::error::Error`.
  
  > +impl From<HgPathError> for std::io::Error {
  > +    fn from(e: HgPathError) -> Self {
  > +        std::io::Error::new(std::io::ErrorKind::InvalidData, e.to_string())
  > +    }
  
  Then, the `e` itself can be embedded in `io::Error`.
  
  > +impl HgPath {
  > +    pub fn new<S: AsRef<[u8]> + ?Sized>(s: &S) -> &Self {
  > +        unsafe { &*(s.as_ref() as *const [u8] as *const Self) }
  > +    }
  
  I moved the declaration of the `struct HgPath` close to this because it's
  so important that `HgPath` is basically a newtype of `[u8]`.
  
  > +    pub fn is_empty(&self) -> bool {
  > +        self.inner.len() == 0
  > +    }
  
  s/.len() == 0/.is_empty()/ in flight.
  
  > +    pub fn join<T: ?Sized + AsRef<HgPath>>(&self, other: &T) -> HgPathBuf {
  > +        let mut inner = self.inner.to_owned();
  > +        if inner.len() != 0 && inner.last() != Some(&b'/') {
  > +            inner.push(b'/');
  > +        }
  > +        inner.extend(other.as_ref().bytes());
  > +        HgPathBuf::from_bytes(&inner)
  > +    }
  
  Perhaps, it's simpler to branch by self.is_empty() first.
  
    if is_empty() {
        other.as_ref().to_owned()
    } else {
        # concatenate
    }
  
  
  
  > +    /// Checks for errors in the path, short-circuiting at the first one.
  > +    /// This generates fine-grained errors useful for debugging.
  > +    /// To simply check if the path is valid during tests, use `is_valid`.
  > +    pub fn check_state(&self) -> Result<(), HgPathError> {
  
  Nit: the word `state` seems confusing. I don't have a good naming skill,
  but I feel `validate()` or `validate_canonical_form()` is better.
  
  > +                b'/' => {
  > +                    if previous_byte.is_some() && previous_byte == Some(b'/') {
  > +                        return Err(HgPathError::ConsecutiveSlashes(
  > +                            bytes.to_vec(),
  > +                            index,
  > +                        ));
  > +                    }
  > +                }
  
  Nit: useless check for previous_byte.is_some()
  
  > +    pub fn push(&mut self, byte: u8) {
  > +        self.inner.push(byte);
  > +    }
  
  
  
  > +impl Extend<u8> for HgPathBuf {
  > +    fn extend<T: IntoIterator<Item = u8>>(&mut self, iter: T) {
  > +        self.inner.extend(iter);
  > +    }
  > +}
  
  `path.push()` and `path.extend(data)` seem weird since they don't care
  the path separator. Can we remove them?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6773/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6773

To: Alphare, #hg-reviewers, kevincox
Cc: mjpieters, yuja, durin42, kevincox, mercurial-devel