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

[WIP] Feature/listp #51

Merged
merged 2 commits into from
Jan 16, 2017
Merged

[WIP] Feature/listp #51

merged 2 commits into from
Jan 16, 2017

Conversation

mfeckie
Copy link
Contributor

@mfeckie mfeckie commented Jan 14, 2017

This follows on from the feature/atom branch, so some of the changes here will disappear once (if) that gets merged.

Had to bump libc because of compiler complaints around panic vs abort

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.

Looking good! A few minor things, but once fixed we can merge :)

@@ -1276,6 +1276,9 @@ CDR_SAFE (Lisp_Object c)
Lisp_Object Fsetcar(Lisp_Object, Lisp_Object);
Lisp_Object Fsetcdr(Lisp_Object, Lisp_Object);
Lisp_Object Fcar(Lisp_Object);
Lisp_Object Fatom(Lisp_Object, Lisp_Object);
Lisp_Object Flistp(Lisp_Object, Lisp_Object);
Lisp_Object Fnlistp(Lisp_Object, Lisp_Object);
Copy link
Collaborator

Choose a reason for hiding this comment

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

These are functions of one argument, which is why the compile is failing on Travis.

@@ -0,0 +1,37 @@
use std::os::raw::c_char;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since lists are just collections of cons cells, I propose that we move these functions into cons.rs. We could move Fatom into cons.rs too.

Slistp,
1, 1,
ptr::null(),
"Return t if OBJECT is a list, that is, a cons cell or nil. Otherwise, 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.

It's probably worth adding (fn OBJECT) at the end of the docstring, or *Help* will just show (listp ARG).

@0xAX
Copy link
Contributor

0xAX commented Jan 14, 2017

Maybe also will be better to move Fatom and Fnull (https://github.com/Wilfred/remacs/blob/master/rust_src/src/strings.rs#L39) to the list.rs as both of these functions related to lists and described as Predicates on Lists in documentation?

@Wilfred
Copy link
Collaborator

Wilfred commented Jan 14, 2017

@0xAX aha, I suggested something similar :). Since we're organising functions by type, I think we should either have a cons.rs or a list.rs, but not both. I don't feel strongly as to which.

@0xAX
Copy link
Contributor

0xAX commented Jan 14, 2017

Since we're organising functions by type, I think we should either have a cons.rs or a list.rs, but not both

fully agree.

I don't feel strongly as to which.

as for me - +1 for list.rs :)

@mfeckie mfeckie changed the title Feature/listp [WIP] Feature/listp Jan 15, 2017
@mfeckie mfeckie closed this Jan 16, 2017
@mfeckie mfeckie reopened this Jan 16, 2017
@mfeckie
Copy link
Contributor Author

mfeckie commented Jan 16, 2017

@Wilfred Finally got back to this, sorry for the delay. Wanted to make sure everything was up to date with master.

Travis is now green

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.

Great, thanks for following up :)

@Wilfred Wilfred merged commit 51d11c4 into remacs:master Jan 16, 2017
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

3 participants