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.

Sign up for GitHub

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

Jump to bottom

Some functions moved to Rust #191

Closed
wants to merge 1 commit into from
Closed

Some functions moved to Rust #191

wants to merge 1 commit into from

Conversation

birkenfeld
Copy link
Collaborator

This is my first venture into remacs, please review carefully. Thanks :)

@jeandudey
Copy link
Contributor

Hi! thanks for the contribution 👍, currently i don't have time to review your PR but i'll do it soon. Also it would be nice if you do a rebase and have a single commit 😄.

@birkenfeld
Copy link
Collaborator Author

Sure, single commit coming up :) I'll also fix the travis run.

@birkenfeld
Move the following Lisp functions to Rust:
6d31022 
* eql
* equal
* equal-including-properties
* delq
* car-safe
* cdr-safe
* memq
* memql
* member
* nthcdr
* nth
* assq
* assoc
* rassq
* rassoc
* plist-get
* plist-put
* lax-plist-put
* lax-plist-get
* plist-member
* string-equal
* markerp

Other changes:

* Create comparison methods for LispObject for each of the three
  ELisp ways (eq, eql, equal).
* Use the provided way to construct bools and t.
* Create a Rusty API for working with cons cells (as_cons() returns
  a newtype that has car() and cdr() methods).
* Introduce a list tails iterator to replace the FOR_EACH_TAIL and
  FOR_EACH_TAIL_SAFE macro.
* Add missing Cargo.lock for remacs-sys.
* Migrate XXXP() functions to LispObject::is_xxx() and remove unused ones.
* Avoid allocating a vector for each call to a MANY function.
* Fixup makefile targets to avoid "override" warnings.
@birkenfeld
Copy link
Collaborator Author

Can't debug the OSX failure, sorry.

@jeandudey jeandudey self-requested a review May 28, 2017 00:17
@birkenfeld birkenfeld mentioned this pull request May 28, 2017
Closed
@Wilfred
Copy link
Collaborator

Wilfred commented May 28, 2017

Awesome! This looks great but I'll give you a proper review shortly. I particularly like your Rust iterator that does the classic lispy hare and tortoise movement through lists.

I don't feel strongly about commit granularity, but I'd prefer that styling and functionality changes were separate at least. No worries about changing this PR though.

I'll have a look at the failing OS X build too.

@Wilfred
Copy link
Collaborator

Wilfred commented May 28, 2017

OK, I've read through the code, and it all looks lovely. 👍

The failure on OS X is just tramp. I'm happy for us to skip the tramp tests on OS X (upstream GNU Emacs doesn't have any CI on OS X to my knowledge).

@birkenfeld how do you want to proceed? I notice that #192 includes these commits.

@Wilfred
Copy link
Collaborator

Wilfred commented May 28, 2017

Looking at the failure in the logs:

Compiling lisp/net/tramp-tests.el
Testing lisp/net/tramp-tests.el
/bin/sh: line 1: 19424 Abort trap: 6           EMACSLOADPATH= LC_ALL=C EMACS_TEST_DIRECTORY=. "../src/remacs" -batch --no-site-file --no-site-lisp -L ":." -l ert -l $loadfile --eval "(ert-run-tests-batch-and-exit (quote (not (tag :expensive-test))))" > lisp/net/tramp-tests.log 2>&1
Running 31 tests (2017-05-27 16:31:23+0000)
Remote directory: `/mock::/var/folders/my/m6ynh3bn6tq06h7xr3js0z7r0000gn/T/'
   passed   1/31  tramp-test00-availability
   passed   2/31  tramp-test01-file-name-syntax
   passed   3/31  tramp-test02-file-name-dissect
   passed   4/31  tramp-test03-file-name-defaults
   passed   5/31  tramp-test04-substitute-in-file-name
   passed   6/31  tramp-test05-expand-file-name
   passed   7/31  tramp-test06-directory-file-name
   passed   8/31  tramp-test07-file-exists-p
   passed   9/31  tramp-test08-file-local-copy
   passed  10/31  tramp-test09-insert-file-contents
   passed  11/31  tramp-test10-write-region
ERROR: lisp/net/tramp-tests.log

This suggests that Remacs is producing SIGABRT on OS X during tramp-test11-copy-file. I suggest we just modify tramp-tests.el to:

(defun tramp--test-enabled ()
  "Whether remote file access is enabled."
  (unless (consp tramp--test-enabled-checked)
    (setq
     tramp--test-enabled-checked
     (cons
      t (ignore-errors
	  (and
           (not (eq system-type 'darwin))
	   (file-remote-p tramp-test-temporary-file-directory)
	   (file-directory-p tramp-test-temporary-file-directory)
	   (file-writable-p tramp-test-temporary-file-directory))))))

  (when (cdr tramp--test-enabled-checked)
    ;; Cleanup connection.
    (ignore-errors
      (tramp-cleanup-connection
       (tramp-dissect-file-name tramp-test-temporary-file-directory)
       nil 'keep-password)))

  ;; Return result.
  (cdr tramp--test-enabled-checked))

jeandudey
jeandudey approved these changes May 29, 2017
View reviewed changes
Copy link
Contributor

@jeandudey jeandudey left a comment

Choose a reason for hiding this comment

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

Everything seems to be correct as Wilfred suggests 👍

@birkenfeld
Copy link
Collaborator Author

Since as you say this is included in #192, I'll just close this one.

I really miss (from Gerrit style reviews) the ability to submit dependent commits and have them reviewed individually.

@birkenfeld birkenfeld closed this May 29, 2017
@birkenfeld birkenfeld deleted the some_functions branch May 29, 2017 04:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Wilfred Wilfred Wilfred approved these changes

@jeandudey jeandudey jeandudey approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@birkenfeld @jeandudey @Wilfred