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

Port "Fcdr()" and "cdr()" to Rust #62

Merged
merged 1 commit into from Jan 15, 2017
Merged

Port "Fcdr()" and "cdr()" to Rust #62

merged 1 commit into from Jan 15, 2017

Conversation

TheKK
Copy link
Contributor

@TheKK TheKK commented Jan 14, 2017

No description provided.

Copy link
Collaborator

@Wilfred Wilfred left a comment

Choose a reason for hiding this comment

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

Apart from a teeny-tiny docstring issue, looks great! :)

cdr(list)
}

defun!("cdr", Fcdr, Scdr, 1, 1, ptr::null(), " Return the cdr of LIST. If arg is nil, return nil.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor nit: could you remove the space before Return?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

@Wilfred Wilfred merged commit 760336e into remacs:master Jan 15, 2017
@TheKK
Copy link
Contributor Author

TheKK commented Jan 15, 2017

Sorry for bringing this up but I'd like to ask some questions about implementation details (hope they're not too trivial):

  1. I saw how Lisp_Cons implemented in C and known there's an union inside it. My question is, how
#[repr(C)]
#[allow(unused_variables)]
struct LispCons {
    car: LispObject,
    cdr: LispObject,
}

#[repr(C)]
#[allow(dead_code)]
pub struct LispConsChain {
    chain: *const LispCons,
}

achieves the same behaviour of C union? Does this mean we'll use mem::transmute to cast LispCons.cdr into LispConsChain.chain?

  1. Is there any concern exposing fn car(object: LispObject) -> LispObject in cons.rs to C code?

  2. In practical, what's the strategy while choosing between

unsafe fn Foo() { ... }

and

fn Foo() { unsafe { } }

@Wilfred
Copy link
Collaborator

Wilfred commented Jan 15, 2017

1: Yep, exactly. We'll use mem::transmute to cast from LispCons.cdr to LispConsChain.cdr. A struct is just a blob of bits, and since they're the same size this is fine.

I'm open to suggestions of cleaner approaches, but I think the current approach is reasonable.

2: car is currently not exposed to the C code, it's a function private to cons.rs. Note that it's not extern "C". We could rename it to CAR and replace the CAR inline function lisp.h. I don't have any concerns with us doing so.

3: See #63, but do let me know if it's unclear.

@TheKK TheKK deleted the cdr branch January 15, 2017 19:19
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.

None yet

2 participants