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 functions interact and which methods have precedence over each other.

In D7116#104769, @Alphare wrote:

The following comparison shows that for input > 20 bytes, fnv has worse overall performance than xx

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 hg.

In D7116#104617, @durin42 wrote:

OOC, have you compared this with the hashbrown crate for perf?

Thanks, looks good.

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.

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 instead of RefCell on those scalar types. I'm not so keen on using UnsafeCell, espacially since the target py_class!-resulting struct could be using RefCell-specific APIs.

It seems weird to add this argument without using it. I would much rather it was added as the implementation was added.

In D6631#97216, @kevincox wrote:

Is there any reason this can't be done using Rc<RefCell<DirsMultiset>>? I have an example here: https://rust.godbolt.org/z/MNNR_F

Is there any reason this can't be done using Rc<RefCell<DirsMultiset>>? I have an example here: https://rust.godbolt.org/z/MNNR_F

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 elements to be exposed through the iterator
interface. Until then, it might be better to move iterator-heavy objects
to CPython/PyO3 layer. Just my guess.

Is the SliceExt change related to the 2018 change? If not could you split the two?

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.

Thanks, looks good.

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 like to push this check down as far as possible.

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.

Thanks for the changes.

In D6231#91102, @gracinet wrote:

I believe to have addressed your immediate concerns.
I played with splitting in different structs for the various stages of the process. It is indeed cleaner and clearer Rust code.
However mutating in place of Enums exposed to Python is a bit of a headache, because of the wrapping in RefCell<Box<T>>, and I don't have a solution for that, so I'm gonna think about that in a follow-up.

In 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.

In 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 compatibility, but I'll play with it a bit.

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 the type system to ensure that you are using the class correctly. However it appears that this is just re-implementing an API so improving that API is probably out of scope for the time being.

In 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.