Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add CursorMut #25

Merged
merged 13 commits into from
Apr 22, 2024
Merged

feat: add CursorMut #25

merged 13 commits into from
Apr 22, 2024

Conversation

olebedev
Copy link
Contributor

@olebedev olebedev commented Feb 25, 2024

Motivation

Add a feature that enables insert elements in the middle of the underlying linked-list. Initial discussion is in #23.

Following further reflection since our initial discussion, I've implemented the CursorMut API as outlined in my previous comment. This pull request is intended to supersede #24, which I suggest we close in favor of this updated approach.

In this PR

The difference to the Cursor API from the RFC:

  • The methods index, splice_*, split_*, as_cursor, and remove_current have been omitted, as they are either not required for my purposes or are not applicable to the context of a LinkedHashMap, such as the index method.

I'm open to discussions about the API design and its implementation. I would appreciate your thoughts on the matter.

Kind regards,
Oleg

@kyren
Copy link
Owner

kyren commented Feb 25, 2024

Hey, I just got back from a trip so this is good timing, I hadn't even had a chance to look at #24 yet. I promise I'll review this the next few days, but from a quick glance, I'm definitely happier with this than I am with something like insert_before. Copying (mostly) the Cursor API from the stdlib is a good move.

It might be nice to have a read-only Cursor as well but that's for sure not required for an initial PR.

/// if the element exists and the operation succeeds, or `false` if the element does not
/// exist.
#[inline]
pub fn move_at(&mut self, key: K) -> bool
Copy link
Contributor Author

@olebedev olebedev Feb 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pub fn move_at(&mut self, key: K) -> bool
pub fn move_at(&mut self, key: K) -> Option<(&K, &mut V)>

This API offers a more user-friendly experience as it provides the key-value in one go. If the caller isn't interested in inspecting or modifying it, they can simply ignore it. Plus, it clearly indicates whether the method call was successful.

What do you think?


Update 1:

Actually, this could be even better?

Suggested change
pub fn move_at(&mut self, key: K) -> bool
pub fn move_at(&mut self, key: &K) -> Option<&mut V>

Because we don't copy the K anymore. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the use of (&K, &mut V), I've observed multiple instances of &mut K being returned by functions. Could this potentially lead to synchronization issues, if the actual key in the node differs from the key used for its insertion into the HashTable?

In my implementation, I've ensured that the key cannot be modified once it's inserted. However, I might be overlooking some aspects. I'd appreciate your assistance in understanding this better.

Copy link
Owner

@kyren kyren Apr 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the use of (&K, &mut V), I've observed multiple instances of &mut K being returned by functions. Could this potentially lead to synchronization issues, if the actual key in the node differs from the key used for its insertion into the HashTable?

These mostly mimic methods that are available in hashbrown, like this one if the behavior of those methods differs from what hashbrown does, it's almost certainly wrong.

I might be mistaken, but are you asking about the &mut Ks being returned from methods in RawVacantEntryMut? Those actually behave the same as hashbrown afaik, but their behavior is not documented that well (or I just can't find it right now). When you receive a RawVacantEntryMut, you receive it for some key that was not found, and it's up to the user to use that RawVacantEntryMut to insert an entry with the same key. If you use RawVacantEntryMut to insert some other key, it will mostly work, except for the fact that you can use this to end up with duplicate entries in the table. The &mut K returned from RawVacantEntryMut methods always refers to a newly inserted entry, because it always inserts a new entry (even if this would result in duplicate entries).

I actually missed this entirely when reviewing #21, I'm ashamed to say, but it is afaict still acceptable behavior for the raw table API. I'm dumb and it has always worked this way, matching what hashbrown does.

@@ -3,7 +3,7 @@ version: 2
jobs:
build:
docker:
- image: cimg/rust:1.65.0
- image: cimg/rust:1.75.0
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The latest version of the ahash library is not compatible with Rust 1.65.0. Therefore, I've updated Rust to 1.75.0.

You can find an example of the build failure here. This was triggered by the previous PR #24, but the issue remains the same. Without a cargo artifacts caching system to maintain a snapshot of dependencies from their initial fetch, I believe the build will also fail on the master branch.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, that was true of ahash 0.8.8 which required 1.72, but 0.8.9 and later relaxed back to 1.60.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, awesome, thanks @cuviper for letting me know, I'll revert this line then.

@kyren
Copy link
Owner

kyren commented Apr 10, 2024

Hi, I'm very sorry I waited so long on reviewing this, I just got through with a huge cross-country move and I was completely out of commission for more than a month while I moved from Florida to Colorado.

@olebedev
Copy link
Contributor Author

Hi @kyren, I understand you've been busy with your move. No worries at all. Please review the PR when the dust settles and you have a moment.

Comment on lines 1816 to 1840
/// Positions the cursor at the element associated with the specified key. Returns `true`
/// if the element exists and the operation succeeds, or `false` if the element does not
/// exist.
#[inline]
pub fn move_at(&mut self, key: &K) -> bool
where
K: Eq + Hash,
S: BuildHasher,
{
unsafe {
let hash = hash_key(self.hash_builder, key);
let i_entry = self
.table
.find_entry(hash, |o| (*o).as_ref().key_ref().eq(key));

match i_entry {
Ok(occupied) => {
let at = *occupied.get();
self.muv(|| at);
true
}
Err(_) => false,
}
}
}
Copy link
Owner

@kyren kyren Apr 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be okay if we dropped move_at for the first version? Is there any compelling reason to have it over dropping the cursor and getting a new one via the entry API?

The reason I feel this way is that not receiving an OccupiedEntry is much harder to mess up than forgetting to check a bool return, and it doesn't seem much worse?

It would be amazing if we could convert the cursor back into a &mut LinkedHashMap too, but that's hard to do with the way the code is currently and that's not necessary for the first version.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can skip it for now. If we have the original map in scope, we can always create it later. However, without the original map in scope, it might pose a problem. WDYT?

It would be amazing if we could convert the cursor back into a &mut LinkedHashMap too, but that's hard to do with the way the code is currently and that's not necessary for the first version.

Absolutely, I've been considering the best approach here. However, I paused before the PR review to avoid making too many assumptions on my own.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can skip it for now. If we have the original map in scope, we can always create it later. However, without the original map in scope, it might pose a problem. WDYT?

If it's a big problem I'd rather be able to go from the cursor back to a &mut LinkedHashMap. This suggestion isn't out of nowhere, it was also made for the current LinkedList CursorMut API: rust-lang/rust#58533 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather be able to go from the cursor back to a &mut LinkedHashMap

@kyren, I'm keen to implement this, as it's required for my use case. Could you advise on the best approach for the implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my understanding, it seems we can't use &mut LinkedHashMap -> CursorMut -> &mut LinkedHashMap due to the double mutable/exclusive borrowing. The only feasible solution appears to be a consuming conversion: LinkedHashMap -> CursorMut -> LinkedHashMap.

Could the move_at method, base on that API, potentially become expensive due to frequent re-allocation caused by this consuming API? Is my understanding correct?

Copy link
Owner

@kyren kyren May 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to understand if a CursorMut is created on the stack each time I convert &mut hashlink::OccupiedEntry to CursorMut

You're asking me if when you create a new CursorMut, is it created on the stack? Yes? How else could it work?

Copy link
Owner

@kyren kyren May 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can see, there's no cost argument at all to be made here, if that's what you're trying to argue. A CursorMut::move_at method is a weird method to provide because it's not any cheaper to do that than to find a new entry and make a cursor out of it, since both of them do the same thing (find an entry from scratch and get the linked list pointer for it). You're presenting an API to the user that makes it seem as though there's some kind of benefit when there's actually not.

It is currently easier to provide CursorMut::move_at due to how the safe hashbrown API works, but you can make both CursorMut and RawOccupiedEntryMut smaller and provide a strictly more flexible API by using the hashbrown raw API with bucket pointers, sidestepping the mutable borrow problem. This is also incidentally exactly the same way hashbrown itself works, if you look at the hash_table::OccupiedEntry, you'll see that it contains a borrow of the parent table and a bucket pointer, which is exactly what I'm proposing: https://docs.rs/hashbrown/latest/src/hashbrown/table.rs.html#1447-1454. It also provides the same API I'm proposing, which is also the same as the one proposed for the LinkedList CursorMut: https://docs.rs/hashbrown/latest/src/hashbrown/table.rs.html#1672-1674

Copy link
Contributor Author

@olebedev olebedev May 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're asking me if when you create a new CursorMut, is it created on the stack? Yes? How else could it work?

No, not really. Could you please clarify if there's a performance difference between the two hypothetical cases:

// 1
let mut cursor = map.cursor_front_mut();
for i in 0..1_000_000_000 {
    match cursor.move_at(&i) {
        Some(_) => cursor.insert_before(new_k, new_v),
        None => todo!(),
    };
};
// 2
for i in 0..1_000_000_000 {
    match map.raw_entry_mut().from_key(&i) {
        RawEntryMut::Occupied(occupied) => {
            let mut cursor = occupied.cursor_mut(); // a new instance of CursorMut has been created
            cursor.insert_before(new_k, new_v);
        },
        RawEntryMut::Vacant(_) => todo!(),
    };
};

In the second scenario, we generate a cursor each time we access the RawEntryMut::Occupied(occupied) => code path. I'm curious if this could impact performance compared to the first scenario where we reuse an existing cursor. Apologies if this question seems basic - I'm still getting acquainted with how the Rust compiler optimizes this.

You're presenting an API to the user that makes it seem as though there's some kind of benefit when there's actually not.

I'm currently trying to understand why my proposal may not be handy as it initially seemed. I'm open for discussion and open to implement your suggestion if there is no difference in performance, once I fully grasp how this part above is workings.

Kind regards,
Oleg

Copy link
Owner

@kyren kyren May 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Speaking practically, there is no performance difference between the two. move_at and from_key both have to do the same work, which is to hash the given key and call HashTable::find to get the pointer to the linked list. Thinking about "creating a CursorMut" itself as having a cost, or trying to "reuse an existing CursorMut" is not usually something that Rust programmers even think about, it's a pointer and a couple of references, optimizing compilers eat this stuff for breakfast. This is why I was so confused before.

Besides this, in my proposed change, the number of references would go from four to one, and then the only thing that would happen is that you would be throwing away the other field cur: *mut Node<K, V> and then recreating it by doing another call to HashTable::find, which is what happens either way. But even this is most likely not going to have much of a (good) performance impact because again, optimizing compilers eat this stuff for breakfast, though it might give the optimizer slightly less work to do.

In some situations there is a very small potential benefit has nothing to do with "reusing" a simple stack variable, it's that creating a CursorMut has to call ensure_guard_node. This doesn't apply to your example though, because you're going through RawOccupiedEntryMut which does not have to do this call. Even if you do end up creating a CursorMut from HashMap::cursor_mut() (which does have to call ensure_guard_node) in a loop, the ensure_guard_node call is almost certainly going to be lifted out of the loop because the compiler will be able to see that self.values gets checked to make sure it's allocated but is never set to NULL within the loop.

Speaking more precisely, optimizing compilers are hard to predict and it's always possible that one or the other loop will be compiled differently, but I don't know which one would be faster or how they'd be different, and if they are measurably different stuff like this is usually considered an "optimization bug". My instincts tell me actually that the worst part of CursorMut is having four references instead of one and that this is worse than any ensure_guard_node call, but it's honestly just a wild guess. When you get down to these sorts of micro-optimizations, usually you just do the simplest, clearest thing unless you have a lot of good evidence to do something else. The API of a library is the primary way that the developer of that library communicates with its users, and having an API which appears to re-use a CursorMut even though there's nothing to re-use feels like a lie, and I feel like the default decision is not to provide that API unless there's some really clear benefit to it.

I'm also emphatically not asking you to do the work of converting to using the hashbrown raw API either, in fact I'm not really in a terrible rush to do this at all (not because of any real downside, but because the raw API is described scarily in terms of stability and as this crate is somewhat foundational, I'm cautious about writing ourselves into a corner if it's removed). In fact, the only way that either move_at or into_map are useful at all is in the rare situation where you have a CursorMut but not the original map (such as in a utility function that takes a CursorMut as a parameter), you can always just drop the CursorMut and re-create it after all. This discussion makes me think that this is actually the situation you're in, but you've convinced yourself that re-using a CursorMut is helpful in some way when it's almost certainly not.

Does this clear things up?

Edit: I didn't cover it, but this also depends on the fact that the entry API for hashbrown::HashTable is not going to be any slower than the non-entry API, which again is generally true as Rust container entry APIs are specifically designed to be zero cost. Whether or not several "zero cost" abstractions all actually add up to zero is sometimes in question unfortunately, but the most useful mental model to have is just to assume that they do until you're presented with evidence that it's actually a problem.

Edit 2: If you want to look at it in another way, why doesn't hashbrown::hash_map::RawOccupiedEntryMut provide a method to reuse the struct and move to another entry?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also emphatically not asking you to do the work of converting to using the hashbrown raw API either, in fact I'm not really in a terrible rush to do this at all (not because of any real downside, but because the raw API is described scarily in terms of stability and as this crate is somewhat foundational, I'm cautious about writing ourselves into a corner if it's removed). In fact, the only way that either move_at or into_map are useful at all is in the rare situation where you have a CursorMut but not the original map (such as in a utility function that takes a CursorMut as a parameter), you can always just drop the CursorMut and re-create it after all.

I don't mind to give the proposed impl a try. I'm unsure yet about how much work it's required though, but using an experimental API doesn't sound ideal to me either. I'd prefer to exclude very rare cases from the library's scope, especially if it's functionality is reachable via existing API with no performance penalty.

This discussion makes me think that this is actually the situation you're in, but you've convinced yourself that re-using a CursorMut is helpful in some way when it's almost certainly not.

You're right. I was about to prevent a scenario where I need to write some boilerplate code for the move-at to functionality, even though it could technically work without it, as you say.

I initially considered it a helper (and this is what I mentioned in my removed comment), but later I've got concerns about the performance impact of not having it. However, your explanation has alleviated those concerns. I trust your judgement on this.

Does this clear things up?

Yep, everything is clear now; thanks for your patience.

Comment on lines 506 to 520

/// Returns the `CursorMut` over the _guard_ node.
///
/// Note: The `CursorMut` over the _guard_ node in an empty `LinkedHashMap` will always
/// return `None` as its current eliment, regardless of any move in any direction.
pub fn cursor_mut(&mut self) -> CursorMut<K, V, S> {
unsafe { ensure_guard_node(&mut self.values) };
CursorMut {
cur: self.values.as_ptr(),
hash_builder: &self.hash_builder,
free: &mut self.free,
values: &mut self.values,
table: &mut self.table,
}
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be more polite to the user to provide cursor_back and cursor_front rather than make them call move_next / move_prev on the returned cursor. I know this isn't strictly necessary but I think it makes it a lot clearer how to use them?

Also eliment -> element.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, it's a valid point. I too was pondering over the same issue, specifically about the cursor's position when we invoke it on an empty instance of LinkedHashMap.

This method is primarily for convenience. I'm open to your suggestions. Could you please share your thoughts on the appropriate behavior for an empty map?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines 1405 to 1410
/// - The current implementation does not include `splice_*`, `split_*`, `as_cursor`, or
/// `remove_current` methods, as there hasn't been a strong demand for these features in
/// real-world scenarios. However, they can be readily incorporated into the existing codebase if
/// needed.
/// - For added convenience, it includes the `move_at` method.
///
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can maybe drop this part of the doc comments?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me.

Comment on lines 1842 to 1846
// Updates the pointer to the current element to the one returned by the at closure function.
#[inline]
fn muv(&mut self, at: impl FnOnce() -> NonNull<Node<K, V>>) {
self.cur = at().as_ptr();
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do the muv and insert helper functions take a closure instead of a normal parameter? They're currently always used like muv(|| at), can it just be muv(at)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good question, I guess the answer is tightly bound to your comment https://github.com/kyren/hashlink/pull/25/files#r1560393496.

The idea behind lazy compute here is that, as you pointed out in your comment, it could be an issue when we insert element before itself, so the lazy computation will help us remove the node from the list, if we found that it's already there. However, I think I haven't implemented that part correctly, I will do.

This is reasonable for the insert function but doesn't make too much sense in case of muv. I initially followed the same style for consistency, but I believe clarity of intention is more important in this particular case where we use unsafe. I'm open to removing it from the muv function for better readability.

Please share your thoughts about the lazy evaluation in insert function.

Comment on lines 1900 to 1901
detach_node(node);
attach_before(node, before());
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is incorrect in the case where you call e.g. insert_before on a cursor where the provided key is the same as the element that the cursor is currently pointing to.

So for example, if you have the map [(2, 2), (3, 3), (4, 4)], and the cursor is pointing to the (3, 3) entry, and you call cursor.insert_before((3, 5)), both node and before() here would be the same node and this would result in a corrupted linked list.

We should add a test for this as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good spot, thank you! Please check out my comment on lazy evaluation here: #25 (comment). I believe my implementation might be incorrect as it doesn't technically provides lazy evaluation, which is necessary to address this issue. I'll be adding a test case for this scenario.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't actually see how lazy evaluation comes into it at all, if you call detach_node(node) and the before node is the same node, you don't have a direct way to insert the node back to where it was because it's been removed from the linked list. I was imagining maybe just a check for equality with before?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're absolutely correct. My apologies for the oversight - the lazy evaluation logic is indeed unrelated to the corrupted linked-list issue.

/// If the entry doesn't exist, it creates a new one. If a value has been updated, the
/// function returns the *old* value wrapped with `Some` and `None` otherwise.
#[inline]
pub fn insert_before(&mut self, tuple: (K, V)) -> Option<V>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of the other methods that insert entries always take the key and value as separate parameters and we should match it here, and the same thing for insert_after.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I'll make that change. Thanks for bringing it to my attention!

Comment on lines 1908 to 1911
self.table.insert_unique(hash, new_node, move |k| {
hash_key(hash_builder, (*k).as_ref().key_ref())
});
attach_before(new_node, before());
Copy link
Owner

@kyren kyren Apr 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one's really pedantic, but I think everywhere else where an insert into the table is actually performed, the node is attached to the linked list before inserting into the table, and it should be the same here.

The reason this matters is for behavior under panics, it saves a memory leak if the hash function panics (the linked list is considered the proper owner of the values rather than the table).

@olebedev olebedev requested a review from kyren April 14, 2024 07:18
Copy link
Contributor Author

@olebedev olebedev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @kyren, the requested changes have been implemented. Please have a look.

If you have any additional suggestions that could enhance this change set, feel free to share. I can incorporate them into this PR or a follow-up one, depending on their size.

Comment on lines 507 to 541
/// Returns the `CursorMut` over the front node.
///
/// Note: The `CursorMut` is pointing to the _guard_ node in an empty `LinkedHashMap` and
/// will always return `None` as its current element, regardless of any move in any
/// direction.
pub fn cursor_front_mut(&mut self) -> CursorMut<K, V, S> {
unsafe { ensure_guard_node(&mut self.values) };
let mut c = CursorMut {
cur: self.values.as_ptr(),
hash_builder: &self.hash_builder,
free: &mut self.free,
values: &mut self.values,
table: &mut self.table,
};
c.move_next();
c
}

/// Returns the `CursorMut` over the back node.
///
/// Note: The `CursorMut` is pointing to the _guard_ node in an empty `LinkedHashMap` and
/// will always return `None` as its current element, regardless of any move in any
/// direction.
pub fn cursor_back_mut(&mut self) -> CursorMut<K, V, S> {
unsafe { ensure_guard_node(&mut self.values) };
let mut c = CursorMut {
cur: self.values.as_ptr(),
hash_builder: &self.hash_builder,
free: &mut self.free,
values: &mut self.values,
table: &mut self.table,
};
c.move_prev();
c
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can still have a cursor_mut method that's private that returns the guard node, to save duplicated code.

Comment on lines 1424 to 1426
/// of its elements. It operates by providing elements as key-value tuples, allowing the value to be
/// modified via a mutable reference while the key could not be changed.
///
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Grammar fix: "could not" -> "cannot"

Or, just something a little simpler: "It provides access to each map entry as a tuple of (&K, &mut V)."

if let linked_hash_map::Entry::Occupied(entry) = map.entry(3) {
entry.cursor_mut().insert_before(3, 5);
let r = map.iter().map(|(k, v)| (*k, *v)).collect::<Vec<_>>();
println!("{r:?}");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stray println?

@kyren
Copy link
Owner

kyren commented Apr 16, 2024

This looks very good, thanks for being patient will all of my requests!

Just a few minor nits and I think it can be merged.

Copy link
Contributor Author

@olebedev olebedev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @kyren, changes are in, please give it another look.

@olebedev olebedev requested a review from kyren April 18, 2024 10:12
@kyren
Copy link
Owner

kyren commented Apr 22, 2024

Looks good now, thank you!

@kyren kyren merged commit 24b87fe into kyren:master Apr 22, 2024
1 check passed
@olebedev olebedev deleted the oleg/add-cursors branch April 26, 2024 07:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants