D3447: rust: make exit handling consistent with `hg`

kevincox accepted this revision. kevincox added inline comments. INLINE COMMENTS > main.rs:204 > +Ok(value) => value, > +Err(_) => panic!("non-integer value returned by > dispatch.run()"), > +}; s/unreachable/panic/ since the code can

D3454: Assign to result from block.

kevincox created this revision. kevincox added a reviewer: indygreg. Herald added subscribers: mercurial-devel, durin42. Herald added a reviewer: hg-reviewers. REVISION SUMMARY This is more idiomatic because you don't have uninitialized variables and has less typing. REPOSITORY rHG

D1581: rust: implementation of `hg`

kevincox added inline comments. INLINE COMMENTS > yuja wrote in main.rs:133 > That's more correct on Unix, but wouldn't work on Windows since > native string is UTF-16 variant (Rust) or ANSI (Python 2.7). Oops, for some reason I thought this was in a unix block. `as_bytes()` isn't actually

D1581: rust: implementation of `hg`

kevincox added a comment. Overall it looks great. I added a bunch of nitpicks. The most common suggestion was to add some contextual information to error messages. Other then that mostly minor style improvements. Feel free to push back on anything you don't agree with :) INLINE COMMENTS >

D2057: translate base85.c into rust code

kevincox added a comment. I agree with the splitting comments :) In fact there might already be a base85 crate which can be used: https://docs.rs/zero85. Either way I'll hold off on the review, feel free to ping me when you are ready for me to take a look. REPOSITORY rHG Mercurial

D2057: rust implementation of hg status

kevincox added a comment. In https://phab.mercurial-scm.org/D2057#43987, @durin42 wrote: > Mercurial tries to be principled about always treating filenames as bytes. AIUI https://www.mercurial-scm.org/wiki/WindowsUTF8Plan is still the plan of record there? Reading that page it

D2057: rust implementation of hg status

kevincox requested changes to this revision. kevincox added a comment. This revision now requires changes to proceed. I will try to finish the review later, but it might be quicker if you incorporate some of the changes first since a lot of them are repeated many times. Overall it looks

D2057: rust implementation of hg status

kevincox added a comment. Rust has platform independent types `PathBuf` and `` for paths and `OsString`

D2057: rust implementation of hg status

kevincox added a comment. I'm not a windows expert but it seems like the rust OsStr, Path and filesystem APIs should handle these conversions for you. I think the only place where you would need to do os-specific code is when doing serialization and serialization which I think should be

D2057: rust implementation of hg status

kevincox added a comment. Ah, I forgot to consider the python interop. Now the need for that crate makes sense. Thanks for explaining. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D2057 To: Ivzhh, #hg-reviewers, kevincox Cc: quark, yuja, glandium, krbullock,

D2057: rust implementation of hg status

kevincox added a comment. The latest changes are looking really good. I have a couple more comments but I didn't have time for a full review. I'll try to get more reviewed tomorrow. It seems that you still have a lot of stuff still in-flight so I'll try to slowly review the changes as I

D5550: rust-cpython: bindings for MissingAncestors

kevincox added inline comments. INLINE COMMENTS > gracinet wrote in ancestors.rs:170 > Hi @kevincox. I have a general question for these, please correct me if I'm > making wrong assumptions. > > Given that we know in advance exactly the number of elements required, and > that it can be large,

D5550: rust-cpython: bindings for MissingAncestors

kevincox added inline comments. INLINE COMMENTS > gracinet wrote in ancestors.rs:170 > Got this one in the wrong direction (sorry about that!) : in this case, this > is a conversion from Rust, so we de have proper `size_hint()` > Still, I'll keep in mind the question for the other direction (in

D5415: rust: changed Graph.parents to return [Revision; 2]

kevincox added a comment. This is a nice improvement. It still seems odd to me to return a list of two parents when there could be 0, 1 or 2 parents. I guess this is for performance reasons? INLINE COMMENTS > ancestors.rs:62 > +this.conditionally_push_rev(parents[0]); > +

D5416: rust: translation of missingancestors

kevincox added inline comments. INLINE COMMENTS > ancestors.rs:154 > +_ => true, > +} > +} Why not just write: self.bases.iter().any(|b| != NULL_REVISION) It is much clearer what you mean and I suspect the performance is very similar. See the below example. If you

D5441: rust-cpython: binding for LazyAncestors

kevincox accepted this revision. kevincox added inline comments. INLINE COMMENTS > ancestors.rs:108 > +Err(e) => { > +return Err(GraphError::new(py, format!("{:?}", e))); > +} .map_err(|e| GraphError::new(py, format!("{:?}", e))) Or even

D5439: rust-cpython: binding for AncestorsIterator

kevincox accepted this revision. kevincox added inline comments. INLINE COMMENTS > ancestors.rs:32 > +Err(_) => 0, // unknown > +}; > +let mut initvec: Vec = Vec::with_capacity(cap); revs.len(py).unwrap_or(0) > ancestors.rs:40 > +Ok(revpy) => revpy.extract(py)?, > +

D5440: rust: core implementation for lazyancestors

kevincox accepted this revision. kevincox added inline comments. INLINE COMMENTS > ancestors.rs:120 > +/// This is mostly meant for iterators backing a lazy ancestors set > +pub fn empty() -> bool { > +if self.visit.len() > 0 { Can you clarify this `is_empty` to match rust

D5441: rust-cpython: binding for LazyAncestors

kevincox accepted this revision. kevincox added inline comments. INLINE COMMENTS > ancestors.rs:98 > +// TODO borrow() can panic, replace with try_borrow() > +// or consider it's ok thanks to the GIL > +AncestorsIterator::from_inner(py, It should be fine with tbe GIL.

D5370: rust: core implementation of missingancestors (no bindings)

kevincox added a comment. I think sticking close to the python version makes sense for the initial version. Then improvements can be made in follow-ups. INLINE COMMENTS > gracinet wrote in lib.rs:26 > The reason I've preferred to implement it directly is that `into_iter()` > iterates on

D5370: rust: core implementation of missingancestors (no bindings)

kevincox added a comment. Overall looks good to me. Just a couple of points. - Using random numbers for tests without a seed that is logged will create failures which are basically impossible to reproduce. - A lot of the comments are comparing to the python, I don't know how useful

D5441: rust-cpython: binding for LazyAncestors

kevincox added a comment. In https://phab.mercurial-scm.org/D5441#81222, @yuja wrote: > Anyway, I just thought using CamelCase in Rust would be slightly nicer and > simpler. If that makes things complicated, I'll drop my idea and stick to > anything that is simpler. I think

D5550: rust-cpython: bindings for MissingAncestors

kevincox added inline comments. INLINE COMMENTS > ancestors.rs:170 > +bases_vec.push(rev.to_py_object(py).into_object()); > +} > +Ok(PyTuple::new(py, bases_vec.as_slice())) This could be a `.collect()` call. Something like: let bases_vec: Vec =

D5944: rust: stop putting NULL_REVISION in MissingAncestors.bases

kevincox accepted this revision. kevincox added inline comments. INLINE COMMENTS > ancestors.rs:245 > self.bases.extend(new_bases); > +self.bases.remove(_REVISION); > } I think it would be more clear if you filtered the NULL_REVISION out before extending the set. let

D5945: rust: itering less on MissingAncestors.bases for max()

kevincox accepted this revision. kevincox added inline comments. INLINE COMMENTS > ancestors.rs:41 > bases: HashSet, > +max_base: Option, > } Does it make sense to just default this to -1 and remove the option? REPOSITORY rHG Mercurial REVISION DETAIL

D5945: rust: itering less on MissingAncestors.bases for max()

kevincox added inline comments. INLINE COMMENTS > gracinet wrote in ancestors.rs:41 > I must confess to have hesitated a bit on that one. On one hand, it would > work, but semantically if ever a new "special" revision -2 is introduced, > this could become problematic. On the other hand, there

D5945: rust: itering less on MissingAncestors.bases for max()

kevincox added inline comments. INLINE COMMENTS > ancestors.rs:347 > } > - > -let max_bases = > -bases_visit.iter().cloned().max().unwrap_or(NULL_REVISION); > -let max_revs = > -revs_visit.iter().cloned().max().unwrap_or(NULL_REVISION); > -

D6231: rust-discovery: starting core implementation

kevincox accepted this revision. kevincox added a comment. I'm not a huge fan of the design of this class because it has a strong protocol to use correctly. It would probably be better to have 3 classes and when you apply new information each class transitions into the next one. This allows

D6230: rust-dagops: roots

kevincox accepted this revision. kevincox added inline comments. INLINE COMMENTS > dagops.rs:96 > +.iter() > +.all(|p| *p == NULL_REVISION || !revs.contains(p)) > +{ I think it is more clear to write `.iter().filter(|p| *p != NULL_REVISION).all(|p|

D6229: rust-dagops: range of revisions

kevincox accepted this revision. kevincox added inline comments. INLINE COMMENTS > dagops.rs:140 > +let l = heads_ancestors.len(); > +for i in 1..=l { > +let rev = heads_ancestors[l - i]; Can you use: for rev in heads_ancestors.into_iter().rev() { // ... } >

D6231: rust-discovery: starting core implementation

kevincox added a comment. In https://phab.mercurial-scm.org/D6231#90744, @gracinet wrote: > That's an interesting idea. I expect it to solve my gripes with `ensure_undecided()` neatly. I'm not sure how it'd fare on the Python side, and probably wouldn't do it in the bindings to maintain

D6230: rust-dagops: roots

kevincox added inline comments. INLINE COMMENTS > gracinet wrote in dagops.rs:96 > Thanks for the suggestion. > > Anticipating on a discussion to have later, I'd like eventually the > `Graph::parents()` returns an iterator. The advantages will be made obvious > by the (not submitted yet)

D6231: rust-discovery: starting core implementation

kevincox added a comment. In https://phab.mercurial-scm.org/D6231#90756, @gracinet wrote: > Yes, that's what I had in mind. Not sure where that enum would be defined (hg-cpython or hg-core) but it doesn't matter so much. I would probably stick it in hg-cpython because it is a

D6231: rust-discovery: starting core implementation

kevincox added inline comments. INLINE COMMENTS > gracinet wrote in discovery.rs:93 > I'm not sure. It wouldn't be a straight error to call this early, as long as > the caller knows what this means, but I don't have any use-case before the > discovery is complete. > > On the other hand, I

D6389: rust-dirstate: create dirstate submodule

kevincox added inline comments. INLINE COMMENTS > mod.rs:6 > +pub p1: &'a [u8], > +pub p2: &'a [u8], > +} If 1 and 2 are the best names why not just make it `pub struct DirstateParetens([&[u8]; 2])`? > mod.rs:7 > +pub p2: &'a [u8], > +} > + It seems odd that this struct is public

D6429: rust-discovery: optimization of add commons/missings for empty arguments

kevincox added a comment. kevincox accepted this revision. Thanks, looks good. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D6429/new/ REVISION DETAIL https://phab.mercurial-scm.org/D6429 To: gracinet, #hg-reviewers, kevincox Cc: durin42,

D6393: rust-dirstate: add "dirs" Rust implementation

kevincox added a comment. kevincox accepted this revision. Sorry. I was busy. In general don't worry about blocking on me, I can't promise any sort of reasonable response time. Worst case I can review the changes after submission and changes can be made afterwards. INLINE COMMENTS >

D6428: rust-discovery: using the children cache in add_missing

kevincox accepted this revision. kevincox added inline comments. INLINE COMMENTS > discovery.rs:233 > +/// sets, for which the discovery process will engage into sampling > +/// that needs the children cache anyway. > pub fn add_missing_revisions( These added comments seem more

D6429: rust-discovery: avoid useless calls to addcommons/addmissings

kevincox requested changes to this revision. kevincox added a comment. This revision now requires changes to proceed. Wouldn't it be better to make `add_missing_revisions` work properly on empty inputs? It seems like it is too error prone to try and catch every caller. If possible I would

D6394: rust-dirstate: add "dirs" rust-cpython binding

kevincox accepted this revision. kevincox added inline comments. INLINE COMMENTS > dirstate.rs:221 > +} > +let dirs_map; > + let dirs_map = if ... Then just let the if statement evaluate to this value. REPOSITORY rHG Mercurial REVISION DETAIL

D6424: rust-discovery: takefullsample() core implementation

kevincox accepted this revision. kevincox added inline comments. INLINE COMMENTS > discovery.rs:124 > +fn next( self) -> Option { > +if self.cur > 1 { > +return None; I think it would be nice to remove the magic number. if self.cur >= self.parents.len() REPOSITORY

D6389: rust-dirstate: create dirstate submodule

kevincox accepted this revision. kevincox added inline comments. INLINE COMMENTS > Alphare wrote in mod.rs:7 > The current Python public API expects `parents` as return value and does > mutate-on-parse for other in-memory objects. Sorry if I'm missing something > obvious. Ok, I'm still not

D6389: rust-dirstate: create dirstate submodule

kevincox added inline comments. INLINE COMMENTS > Alphare wrote in mod.rs:7 > This is public so that `hg-cpython` can use it. I think it helps with > readability too, a "bag of bytes" is sometimes not really helpful when > stumbling on code. I guess the point I am trying to make is why do we

D6345: rust-discovery: optionally don't randomize at all, for tests

kevincox accepted this revision. kevincox added a comment. The code and rust style looks fine. I'm not sure that taking the N smallest elements is the best approach for testing though. I can imagine it hiding some issues with larger values. REPOSITORY rHG Mercurial REVISION DETAIL

D6348: rust-dirstate: add rust implementation of `parse_dirstate` and `pack_dirstate`

kevincox requested changes to this revision. kevincox added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > dirstate.rs:16 > +pub type DirstateVec = Vec<(Vec, DirstateTuple)>; > +pub type CopyMap<'a> = Vec<(&'a [u8], &'a [u8])>; > + You have a lot of tuples

D6257: rust-discovery: core implementation for take_quick_sample()

kevincox accepted this revision. kevincox added inline comments. INLINE COMMENTS > discovery.rs:70 > +}; > +if seen.contains() { > +continue; You can do if see.insert(current).is_some() { continue; // Already present. } And avoid the insert below.

D6271: rust-filepatterns: add a Rust implementation of pattern-related utils

kevincox requested changes to this revision. kevincox added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > filepatterns.rs:18 > +}; > +} > + Why not use https://docs.rs/regex/1.1.6/regex/fn.escape.html? If you are worried about performance you can use

D6594: RFC dirstatemap

kevincox added inline comments. INLINE COMMENTS > dirstate_map.rs:47 > + > +impl Default for DirstateMap { > +fn default() -> Self { Can you just `#[derive(Default)]`? > dirstate_map.rs:95 > +filename: &[u8], > +old_state: u8, > +entry: DirstateEntry, This should

D6393: rust-dirstate: add "dirs" Rust implementation

kevincox added inline comments. INLINE COMMENTS > martinvonz wrote in dirs_multiset.rs:43 > Maybe Kevin meant something like this? > > if skip_state == None | skip_state == Some(state) { > multiset.add_path(filename); > } No, you are right. I misread the condition. REPOSITORY rHG

D6597: rust-2018: switch hg-core and hg-cpython to rust 2018 edition

kevincox added a comment. kevincox accepted this revision. Is the SliceExt change related to the 2018 change? If not could you split the two? INLINE COMMENTS > utils.rs:3 > + > +pub fn replace_slice(buf: [T], from: &[T], to: &[T]) > +where This could really use a doc comment. >

D6593: rust-minor-fixes: remove Deref in favor of explicit methods

kevincox added inline comments. INLINE COMMENTS > dirs_multiset.rs:131 > + > +pub fn iter() -> Iter, u32> { > +self.inner.iter() Returning a `std::collections::hash_map::Iter` binds you to this implementation. I would recommend instead returning `impl Iterator`. REPOSITORY rHG

D6594: RFC dirstatemap

kevincox added a comment. In D6594#96518 , @yuja wrote: > Yes, so we'll probably want to minimize the data exchanged between Rust > and Python. This means we'll eventually move more logic to Rust side, which > hopefully reduce the number of

D6626: rust-dirstate: create dirstate submodule in hg-cpython

kevincox added inline comments. kevincox accepted this revision. INLINE COMMENTS > dirs_multiset.rs:34 > +skip_state = Some(skip.extract::(py)?.data(py)[0] as > i8); > +} > +let dirs_map; let skip_state = skip.map(|skip| skip.extract::(py)?.data(py)[0] as i8); >

D6231: rust-discovery: starting core implementation

kevincox added a comment. In https://phab.mercurial-scm.org/D6231#91102, @gracinet wrote: > I believe to have addressed your immediate concerns. > > I played with splitting in different `struct`s for the various stages of the process. It is indeed cleaner and clearer Rust code. >

D6260: rust-discovery: takefullsample() core implementation

kevincox accepted this revision. kevincox added inline comments. INLINE COMMENTS > discovery.rs:108 > +cur: usize, > +} > + You might also consider an enum enum Parents { Zero, One(Revision), Two(Revision, Revision), } This also makes the iterator implementation a little

D6271: rust-filepatterns: add a Rust implementation of pattern-related utils

kevincox accepted this revision. kevincox added a comment. Thanks for the changes. INLINE COMMENTS > Alphare wrote in filepatterns.rs:18 > I think I hit the same problem as the person in this issue: > `https://github.com/rust-lang/regex/issues/451`. It's a hassle to escape > bytes using

D6631: rust-cpython: add macro for sharing references

kevincox added a comment. Is there any reason this can't be done using `Rc>`? I have an example here: https://rust.godbolt.org/z/MNNR_F It uses `Rc` and `RefCell` however it does need to do some unsafe work to keep the RefCell borrowed for longer than would otherwise be easy to do. The

D6631: rust-cpython: add macro for sharing references

kevincox added a comment. In D6631#97216 , @kevincox wrote: > Is there any reason this can't be done using `Rc>`? I have an example here: https://rust.godbolt.org/z/MNNR_F I see now that since python is managing the reference it does make

D6631: rust-cpython: add macro for sharing references

kevincox added a comment. kevincox accepted this revision. In D6631#97887 , @Alphare wrote: > @kevincox I've renamed the file since it no longer just contains macros. > I've moved whatever I could move to separate structs and used `Cell`

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

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 >

D6765: rustfilepatterns: shorter code for concatenating slices

kevincox added inline comments. kevincox accepted this revision. INLINE COMMENTS > filepatterns.rs:161 > } > -let mut res = b".*".to_vec(); > -res.extend(pattern); > -res > +[".*"[..], pattern].concat() > } I don't think you

D6765: rustfilepatterns: shorter code for concatenating slices

kevincox added inline comments. INLINE COMMENTS > valentin.gatienbaron wrote in filepatterns.rs:161 > Indeed, although it took a compiler upgrade. > I don't really understand why the compiler accepts the change here but not in > the other places. It seems a bit random to remove the borrow only

D6766: rustfilepatterns: refactor the pattern of removing a prefix from a &[u8]

kevincox added inline comments. kevincox accepted this revision. INLINE COMMENTS > utils.rs:44 > fn trim() -> > +fn chop_prefix(, needle:) -> Option<>; > } I have a small preference for `drop_prefix`. > utils.rs:44 > fn trim() -> > +fn chop_prefix(, needle:) -> Option<>; >

D6774: rust-hgpath: replace all paths and filenames with HgPath/HgPathBuf

kevincox added inline comments. kevincox accepted this revision. INLINE COMMENTS > files.rs:75 > +/// TODO more than ASCII? > +pub fn normalize_case(path: ) -> HgPathBuf { > +#[cfg(windows)] // NTFS compares via upper() Is this used? Can you either use, delete or add tests. REPOSITORY

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

kevincox added inline comments. kevincox accepted this revision. INLINE COMMENTS > hg_path.rs:175 > +pub fn push( self, byte: u8) { > +self.inner.push(byte); > +} debug_assert check after the mutation. > hg_path.rs:242 > +fn extend>( self, iter: T) { > +

D6628: rust-parsers: switch to parse/pack_dirstate to mutate-on-loop

kevincox added inline comments. kevincox accepted this revision. INLINE COMMENTS > mod.rs:16 > +pub p1: Vec, > +pub p2: Vec, > } If these are always 20 bytes long you are best to use `[u8; 20]`. That way there will be no indirection. And since `Vec` is 3 words it will have a bigger

D6629: rust-dirstate: use EntryState enum instead of literals

kevincox added inline comments. INLINE COMMENTS > Alphare wrote in lib.rs:85 > Did you mean `{:?}`. I can't find anything related to your syntax. Sorry yes. I've been writing too much python at work. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION

D6629: rust-dirstate: use EntryState enum instead of literals

kevincox added inline comments. kevincox accepted this revision. INLINE COMMENTS > lib.rs:85 > +Overflow => "Overflow in dirstate.".to_string(), > +CorruptedEntry(e) => format!("Corrupted entry: {}.", e), > +Damaged => "Dirstate appears to be

D6632: rust-dirstate: rust implementation of dirstatemap

kevincox added inline comments. kevincox accepted this revision. INLINE COMMENTS > dirstate_map.rs:191 > +changed = true; > +*entry = DirstateEntry { > +mtime: MTIME_UNSET, Can you just do `entry.mtime = MTIME_UNSET`?

D6516: rust-discovery: accept the new 'respectsize' init arg

kevincox added a comment. kevincox accepted this revision. It seems weird to add this argument without using it. I would much rather it was added as the implementation was added. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D6516/new/ REVISION

D7299: rust-status: return a ParallelIterator instead of a Vec from stat_dmap_entries

kevincox added inline comments. kevincox accepted this revision. INLINE COMMENTS > status.rs:128 > +Err(e) => return Err(e.into()), > +}; > This can be replaced with: let meta = root_dir.as_ref().join(hg_path_to_path_buf(filename)?).symlink_metadata(); Although I

D7300: rust-status: refactor dispatch case for normal files

kevincox added inline comments. kevincox accepted this revision. INLINE COMMENTS > status.rs:31 > > #[inline] > +/// Dates and times that are outside the 31-bit signed range are compared I see a lot of these and in general I would recommend against it. The compiler is smart. Unless

D7254: rust-status: improve status performance

kevincox added inline comments. kevincox accepted this revision. INLINE COMMENTS > status.rs:41 > +last_normal_time: i64, > +) -> (HgPathBuf, Dispatch) { > +let filename = filename.as_ref(); Every exit path is just `filename.to_owned()`. It would be better to just let the caller do

D7178: [RFC] rust-matchers: add `Matcher` trait and implement `AlwaysMatcher`

kevincox added a comment. I see a lot of the functions are here to give optimization hints. In order to make someone non-familiar with the code able to understand it each method should state the contracts that it is making. I am having a really hard time reconciling how the different

D7577: hg-core: implement Mercurial's config file discovery logic

kevincox added inline comments. kevincox accepted this revision. INLINE COMMENTS > config.rs:145 > +let paths = glob_res > +.filter_map(|x| if let Ok(x) = x { Some(x) } else { None }) > +.sorted(); I believe this can just be `.flatten()`. REPOSITORY

D7604: rust-hg-path: implement more readable custom Debug for HgPath{, Buf}

kevincox added inline comments. kevincox accepted this revision. INLINE COMMENTS > hg_path.rs:186 > +fn fmt(, f: fmt::Formatter<'_>) -> fmt::Result { > +write!(f, "HgPath({})", String::from_utf8_lossy()) > +} I would recommend `"HgPath({:?})"` so that the result is unambiguous

D7116: rust-performance: introduce FastHashMap type alias for HashMap

kevincox added a comment. In D7116#104769 , @Alphare wrote: > The following comparison shows that for input > 20 bytes, fnv has worse overall performance than xx Sounds good. We can always do benchmarks in the future and swap it.

D7058: rust-dirstate-status: add first Rust implementation of `dirstate.status`

kevincox added a comment. kevincox accepted this revision. Thanks, looks good. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7058/new/ REVISION DETAIL https://phab.mercurial-scm.org/D7058 To: Alphare, #hg-reviewers, kevincox Cc: yuja, martinvonz,

D7116: rust-performance: introduce FastHashMap type alias for HashMap

kevincox added a comment. In D7116#104617 , @durin42 wrote: > OOC, have you compared this with the hashbrown crate for perf? std::collections::HashMap now uses hashbrown https://blog.rust-lang.org/2019/07/04/Rust-1.36.0.html REPOSITORY

D7116: rust-performance: introduce FastHashMap type alias for HashMap

kevincox added a comment. kevincox accepted this revision. I've seen very good results with https://github.com/servo/rust-fnv in the past so it is probably worth including that in the comparison and possibly using it. It is especially good for small keys which seems like a common case in

D7058: rust-dirstate-status: add first Rust implementation of `dirstate.status`

This revision now requires changes to proceed. kevincox added inline comments. kevincox requested changes to this revision. INLINE COMMENTS > Alphare wrote in status.rs:83 > Collecting into a `Result` prevents us from doing a `filter_map` during > `collect`. Implementing a `ParallelIterator`

D7525: rust-matchers: improve `Matcher` trait ergonomics

kevincox added inline comments. kevincox accepted this revision. INLINE COMMENTS > matchers.rs:87 > +None > } > fn exact_match(, _filename: impl AsRef) -> bool { Another option here would just to have a global empty hash set. REPOSITORY rHG Mercurial CHANGES SINCE LAST

D7523: rust-hg-path: implement `Display` for `HgPath` and `HgPathBuf`

This revision now requires changes to proceed. kevincox added inline comments. kevincox requested changes to this revision. INLINE COMMENTS > hg_path.rs:168 > +fn fmt(, f: fmt::Formatter<'_>) -> fmt::Result { > +write!(f, "HgPath {{ {} }}", String::from_utf8_lossy()) > +} This

D7529: rust-dirstate-status: add `walk_explicit` implementation, use `Matcher` trait

This revision now requires changes to proceed. kevincox added inline comments. kevincox requested changes to this revision. INLINE COMMENTS > status.rs:125 > +/// TODO subrepos > +fn walk_explicit<'a: 'c, 'b: 'c, 'c>( > +files: &'a HashSet<>, It seems to me that you can just use one

D7526: rust-hg-path: add method to get part of a path relative to a prefix

This revision now requires changes to proceed. kevincox added inline comments. kevincox requested changes to this revision. INLINE COMMENTS > hg_path.rs:131 > +let base = base.as_ref(); > +if base.len() == 0 { > +return Some(self); `base.is_empty()`? If we don't have

D8110: rust-dirstatemap: cache non normal and other parent set

kevincox added inline comments. INLINE COMMENTS > dirstate_map.rs:38 > +non_normal_set: Option>, > +other_parent_set: Option>, > parents: Option, I don't understand why an `Option` is faster than a `HashSet`. Could you add some explanation to the commit message? Is this to avoid

D8048: rust-dirstatemap: cache non normal and other parent set

kevincox added inline comments. INLINE COMMENTS > dirstate_map.rs:251 > +self.set_non_normal_other_parent_entries(false); > +( self.non_normal_set, self.other_parent_set) > +} If we have just set the fields to `Some(..)` in the previous line can't we do the unwrap here

D7927: rust-status: add util for listing a directory

kevincox added inline comments. kevincox accepted this revision. INLINE COMMENTS > status.rs:76 > + > +results.sort_by(|a, b| a.0.cmp()); > +Ok(results) It would be more clear to do `.sort_by_key(|e| e.0)` And I don't think stability matters here so you could use

D7929: rust-status: add bare `hg status` support in hg-core

This revision now requires changes to proceed. kevincox added inline comments. kevincox requested changes to this revision. INLINE COMMENTS > status.rs:201 > root_dir: impl AsRef + Sync + Send, > +work: mpsc::Sender<&'a HgPath>, > options: StatusOptions, Please describe this

D7797: rust-nodemap: pure Rust example

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 `//!`

D7927: rust-status: add util for listing a directory

kevincox added inline comments. INLINE COMMENTS > Alphare wrote in status.rs:76 > I didn't realize that `sort_unstable_by_key` needed to return a `T` and not a > ``, that forces a `clone()`. I'm not sure that matters, we'll go with the > shortest code and see if that shows up in profiling

D7796: rust-nodemap: input/output primitives

kevincox added inline comments. kevincox accepted this revision. INLINE COMMENTS > nodemap.rs:191 > fn new() -> Self { > -Block([-1; 16]) > +Block([255u8; BLOCK_SIZE]) > } A comment why 255 is a good number would be nice. > nodemap.rs:334 > +// Assert that

D8098: rust-nodemap: a method for full invalidation

kevincox added inline comments. kevincox accepted this revision. INLINE COMMENTS > nodemap.rs:577 > +/// immutable part. > +pub fn invalidate_all( self) -> () { > +self.root = Block::new(); `-> ()` is unnecessary. > nodemap.rs:580 > +self.growable = Vec::new(); > +

D7921: rust-dirs-multiset: improve temporary error message

This revision now requires changes to proceed. kevincox added inline comments. kevincox requested changes to this revision. INLINE COMMENTS > dirs_multiset.rs:86 > +subpath.as_bytes(), > +); > +let second_slash_index = offset.unwrap() +

D7796: rust-nodemap: input/output primitives

kevincox added inline comments. INLINE COMMENTS > Alphare wrote in nodemap.rs:268 > @kevincox @durin42: I've changed the code to use the type-level > `ManuallyDrop` instead of `mem::forget`, added the `transmute` to be extra > paranoid and added a comment. This doesn't address the primary

D8110: rust-dirstatemap: cache non normal and other parent set

kevincox added inline comments. INLINE COMMENTS > Alphare wrote in dirstate_map.rs:251 > `as_mut()` only does ` Option -> Option< T>`, which isn't always > what we want (however you can argue that it would save a few keystrokes), but > calling `unwrap()` circles back to the same `error[E0515]:

D7797: rust-nodemap: pure Rust example

kevincox added inline comments. kevincox accepted this revision. INLINE COMMENTS > Alphare wrote in index.rs:76 > I'm not sure of the point of doing so with an mmap. We have to trust what the > mmap is giving us. In general an assertion is better than a comment because it expresses the things

D7796: rust-nodemap: input/output primitives

kevincox added inline comments. INLINE COMMENTS > Alphare wrote in nodemap.rs:268 > I think that would be sufficient indeed. How about making `Block` into a > `[u8; 64]`? It will be less expressive, but this will no longer be needed. I like that. It makes it much more explicit that it is a

D7797: rust-nodemap: pure Rust example

kevincox added inline comments. INLINE COMMENTS > Alphare wrote in index.rs:76 > I am not sure exactly what the `debug_assert` should be? Do you want to check > that the `len` is a multiple of the `align_of::`? Yes, that seems reasonable. It shows what data you are ignoring and will alert us

D7923: rust-matchers: add functions to get roots, dirs and parents from patterns

kevincox added a comment. Thanks for the heads up. I'll try to dedup them. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7923/new/ REVISION DETAIL https://phab.mercurial-scm.org/D7923 To: Alphare, #hg-reviewers, kevincox Cc: durin42, kevincox,

  1   2   >