kevincox added inline comments. kevincox accepted this revision. INLINE COMMENTS
> index.rs:6 > + > +/// Minimal `RevlogIndex`, readable from standard Mercurial file format > +use hg::*; Since this is a file-level comment it should use `//!` https://doc.rust-lang.org/reference/comments.html#examples > index.rs:47 > + if i >= self.len() { > + None > + } else { I would make this an early return and remove the else. It seems to me like an unexpected condition. If it should never happen then please add a debug_assert as well. > index.rs:76 > + let ptr = self.0.as_ptr() as *const IndexEntry; > + // Any misaligned data will be ignored. > + unsafe { slice::from_raw_parts(ptr, self.0.len() / INDEX_ENTRY_SIZE) > } Could you add at least a debug_assert that the alignment is correct? > index.rs:84 > + let file = File::open(path).unwrap(); > + let mmap = unsafe { MmapOptions::new().map(&file).unwrap() }; > + Self { Please put the `unsafe` block around only the unsafe operation. This makes it more obvious what I should look at. let mmap = unsafe { MmapOptions::new().map(&file) }.unwrap(); > index.rs:84 > + let file = File::open(path).unwrap(); > + let mmap = unsafe { MmapOptions::new().map(&file).unwrap() }; > + Self { It seems like we should handle this error. At the very least we should be providing context such as "your index file {:?} is missing". > main.rs:8 > +extern crate hg; > +extern crate memmap; > + We should be using Rust 2018 so externs aren't required. > main.rs:41 > +fn create(index: &Index, path: &Path) -> io::Result<()> { > + let mut file = File::create(path)?; > + let start = Instant::now(); Do you create this at the start to avoid the work if you don't have permission? If not it seems to be that it would be better to avoid creating the file if creating the nodemap fails. Otherwise we are leaving an empty file around. If so please document your reasoning. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7797/new/ REVISION DETAIL https://phab.mercurial-scm.org/D7797 To: gracinet, #hg-reviewers, marmoute, kevincox Cc: marmoute, durin42, kevincox, mercurial-devel _______________________________________________ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
