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

[kevincox (Kevin Cox)]

This revision now requires changes to proceed.
kevincox added a comment.
kevincox requested changes to this revision.


  It looks good overall. I just would like to have a bit more strict definition of what an HgPath can contain and preferably some init-time validation.

INLINE COMMENTS

> hg_path.rs:19
> +/// decoded from MBCS to WTF-8. If WindowsUTF8Plan is implemented, the source
> +/// character encoding will be determined per repository basis.
> +#[derive(Eq, Ord, PartialEq, PartialOrd, Debug, Hash)]

You should probably add a note about what a valid path is. My initial assumption is "A sequence of non-null characters. `/` separates directories, consecutive `/` are forbidden". It would be good to write this down, provide an `is_valid` function and add a debug_assert! on creation/modification to check that everything is as expected.

> hg_path.rs:43
> +    }
> +    pub fn iter(&self) -> HgPathIterator {
> +        HgPathIterator { path: &self }

Please call this `bytes()` to highlight that it iterates over bytes, not characters or directory components.

> hg_path.rs:136
> +#[derive(Debug)]
> +pub struct HgPathIterator<'a> {
> +    path: &'a HgPath,

I would also call this `HgPathByteIterator`

> hg_path.rs:204
> +
> +    #[inline]
> +    fn deref(&self) -> &HgPath {

This is almost certainly unnecessary.

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: durin42, kevincox, mercurial-devel