Port kill-buffer #1507
Port kill-buffer #1507
Conversation
|
As this PR hasn't seen activity for quite some time I've taken the liberty of continuing it |
|
Oh, wow, thank you! This PR fell off my radar as I've been busy with work. I'd love to get back onto it, but I doubt that's realistic any time soon. If you can push it forwards, that'd be awesome. Sorry for leaving it dangling. |
|
That's quite all right! Everyone has obligations from time to time 🙂. And there's a solid amount of groundwork to build on, so finishing it won't be too difficult. |
Iterator types and various other tweaks have been added where required to achieve this
|
This function is somewhat long, I've managed to stare myself blind reading it over and over. I've used the rust versions of functions where their C exports were previously used, and I've added some iterator types to make some iterations idiomatic. There are a couple very simple C functions that are invoked that could potentially be ported as well, but I refrained from doing so to keep things from blowing too far out of scope. |
rust_src/src/buffers.rs
Outdated
| // We do it at this stage so nothing terrible happens if they | ||
| // ask questions or their hooks get errors. | ||
| if b.base_buffer.is_null() && b.indirections > 0 { | ||
| for other in LispBufferIter::new() { |
There was a problem hiding this comment.
I like the idea of this iterator. But maybe we can come up with a name for the iteration over the global buffer list instead of the Iterator automatically walking that list.
There was a problem hiding this comment.
So something like renaming the constructor from new to something like all or global_list or from_global?
There was a problem hiding this comment.
Ideally the LispBufferIter constructor could be given a list of buffers and there would be a function or constructor/factory that would give the iterator over the global list.
|
@agraven thanks for picking this up. This function is a great example of the meandering, old-school C code. When we can refactor all of these into idiomatic code it will be nice nice nice. |
Improved readability of matching on function argument, added wrapper for accessing minibuf_window, removed unnecessary parts of c code
Rename the LispBufferIter constructor for iterating all buffers to all_buffers, and add a constructor for starting at an arbitrary point. Reflow comments in function, correct grammar, and clarify where needed. Reduce the area marked as unsafe in the area regarding resetting local variables.
| pub fn all_buffers() -> Self { | ||
| unsafe { | ||
| Self { | ||
| current: LispBufferRef::from_ptr(all_buffers as *mut c_void), |
There was a problem hiding this comment.
Note that the constructor iterating all buffers doesn't rely on the one for starting at a specified point, as I felt safer accounting for the possibility of all_buffers being null.
|
Whoops, didn't realize rustfmt wasn't set up correctly |
A somewhat literal translation of kill-buffer to Rust.
There's a lot of unsafe code, including plenty of pointer work on linked lists. Compared to e.g. accessors, I don't see a particularly pleasant way to wrap up the modifications to these structures to hide the pointer-handling away in reusable helper functions (at least, that doesn't feel too forced).
I think useful next steps would be to port the other (no longer static) functions called by kill-buffer over to Rust, and perhaps break apart kill_buffer into smaller functions rather than one giant do-everything function, but I'd appreciate thoughts on what we have so far before going further.