-
Notifications
You must be signed in to change notification settings - Fork 73
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
[WIP] Add buffer and thread modules #152
base: master
Are you sure you want to change the base?
Conversation
rust_src/crates/lisp/src/eval.rs
Outdated
/// Thus, (funcall \\='cons \\='x \\='y) returns (x . y). | ||
/// usage: (funcall FUNCTION &rest ARGUMENTS) | ||
#[allow(unused_assignments)] | ||
pub fn funcall(args: &mut [LispObject]) -> LispObject { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's my take on this - I think we will get ourselves into trouble long term if we copy too much of remacs re-implementations. I think copying remacs wrapper classes, and simple one liners (like intern below) are good, but larger functions like funcall will cause issues. Instead, I think it would be better if we made these functions 'safe wrappers' around the native C functions - that way we keep the implementations aligned, but we are still writing idiomatic Rust and aren't littering the code with unsafe and as_mut_ptr()
calls .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I was also hesitant to do this. I'll look for an alternative.
@DavidDeSimone I removed funcall. |
Oh I forgot that I still need to find an alternative for funcall in call =) |
pub struct ThreadState {} | ||
|
||
impl ThreadState { | ||
pub fn current_buffer_unchecked() -> LispBufferRef { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since this is unchecked
do we want to mark it unsafe? What it is doing seems pretty unsafe to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say no. It doesn't help anyone.
Usually marking a function unsafe
communicates to the caller that they need to enforce a certain invariant or they will cause UB. You're not meant to tinker with the current_thread_pointer
anyway, so safe rust will not cause UB in this function.
The same goes for the other function.
This is some code that I need for git stuff. The remacs macro
call
combined withintern
are good for testing purposes.It seems they removed
check_cons_list
fromfuncall
upstream, does anybody see why ?