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

Implement `lisp_fn` procedural macro #181

Merged
merged 6 commits into from May 28, 2017

Conversation

Projects
None yet
2 participants
@jeandudey
Collaborator

jeandudey commented Apr 22, 2017

Status: Not ready for review
Signed-off-by: Jean Pierre Dudey jeandudey@hotmail.com

@@ -0,0 +1,42 @@
[root]

This comment has been minimized.

@jeandudey

jeandudey Apr 22, 2017

Collaborator

Oops, this shouldn't be here, I'll delete it later.

@jeandudey

This comment has been minimized.

Collaborator

jeandudey commented Apr 22, 2017

Check list:

  • Better error handling, there are a lots of unwraps around there, at least they should be formatted.
  • Detect MANY arguments, the macro should check for &[LispObject].
  • Put the Rust function back in code.
  • Detect if the arguments are LispObject or &[LispObject]
    • Detect if &[LispObject] and LispObject are mixed and throw an error.
  • Remove that debug panic at the end.
  • Add a README.md in remacs-macros explaining what lisp_fn does and how it behaves when different arguments are passed.
  • Configure .travis.yml to use nightly
Implement `lisp_fn` procedural macro
Signed-off-by: Jean Pierre Dudey <jeandudey@hotmail.com>

@jeandudey jeandudey changed the title from [WIP] Implement `lisp_fn` procedural macro to Implement `lisp_fn` procedural macro Apr 23, 2017

@jeandudey

This comment has been minimized.

Collaborator

jeandudey commented Apr 23, 2017

@Wilfred now ready to review, i don't have implemented support for UNEVALLED functions, it should be trivial to do but i left it as part to another PR.

@Wilfred

OK, I've gone through and reviewed all of this.

The API is really nice, I like it 👍

Left some comments, but this is definitely a good approach. :)

### `MANY` functions
This attributes handles the definitions of `MANY` functions, these functions can take an arbitrary number of arguments, but you *still can* specify a `min` number of arguments.

This comment has been minimized.

@Wilfred

Wilfred Apr 24, 2017

Owner

*This attribute

But I'm not sure what attribute you're referring too here. Do we need to use the term MANY? I think it's just a term the Emacs C uses, but readers of the README might not know it.

## `lisp_fn`
This macros creates the necessary functions and symbols automatically.
It handles `MANY` functions and normal functions too (support for `UNEVALLED`

This comment has been minimized.

@Wilfred

Wilfred Apr 24, 2017

Owner

I think the note about UNEVALLED belongs in a comment rather than in the docs that new users see first.

# Attributes
## `lisp_fn`
This macros creates the necessary functions and symbols automatically.

This comment has been minimized.

@Wilfred

Wilfred Apr 24, 2017

Owner

*This macro

```rust
/// Return the same argument
#[lisp_fn(name = "same", min = "1")]

This comment has been minimized.

@Wilfred

Wilfred Apr 24, 2017

Owner

Do we support setting a maximum number of arguments in this macro? We probably should, so we can use it in this example.

This comment has been minimized.

@jeandudey

jeandudey Apr 25, 2017

Collaborator

The maximum number of arguments are calculated automatically counting the number of arguments in the function.

This comment has been minimized.

@Wilfred

Wilfred May 7, 2017

Owner

Aha, excellent. We should probably mention this.

Where the `name` argument specifies the **symbol name** that is going to be use in Emacs Lisp, `min` specifies the **minimum** number of arguments that can be passed to this function.
In this example the attribute generates the `Fsame` function that is going to b called in **C**, and the `Ssame` structure that holds the function information. You still need to register the function with `defsubr` to make it visible in Emacs Lisp. To make a function visible to C you need to export it in the crate root (lib.rs) as follows:

This comment has been minimized.

@Wilfred

Wilfred Apr 24, 2017

Owner

*going to be

}
```
## `lisp_doc`

This comment has been minimized.

@Wilfred

Wilfred Apr 24, 2017

Owner

I don't think we should have code that doesn't work. Let's just drop this and leave the docs as Rust docstrings.

@@ -1,8 +1,6 @@
language: rust
cache: cargo
rust:
- stable
- beta
- nightly

This comment has been minimized.

@Wilfred

Wilfred Apr 24, 2017

Owner

We should probably also test against a pinned nightly version too, so we have a known-good point in time to compare with when nightly changes.

This comment has been minimized.

@jeandudey

jeandudey May 20, 2017

Collaborator

I think we should only use one nightly because the proc-macro doesn't change that much, also it would slow builds

}
if !is_rust_abi(abi) {
return Err("lisp functions can only use \"Rust\" ABI");

This comment has been minimized.

@Wilfred

Wilfred Apr 24, 2017

Owner

Nice defensive coding :)

max_args: #max_args,
symbol_name: (#symbol_name).as_ptr() as *const ::libc::c_char,
intspec: ::std::ptr::null(),
doc: ::std::ptr::null(),

This comment has been minimized.

@Wilfred

Wilfred Apr 24, 2017

Owner

It's annoying that Emacs doesn't look at this attribute in the struct. We should probably fix that in Remacs (not in this PR).

use remacs_sys::{EmacsInt, CHARACTERBITS};
/// Maximum character code
pub const MAX_CHAR: EmacsInt = (1 << CHARACTERBITS as EmacsInt) - 1;
#[lisp_fn(name = "max-char")]

This comment has been minimized.

@Wilfred

Wilfred Apr 24, 2017

Owner

This is a really nice API, I like it.

Add missing use declaration
Signed-off-by: Jean Pierre Dudey <jeandudey@hotmail.com>
@jeandudey

This comment has been minimized.

Collaborator

jeandudey commented May 20, 2017

@Wilfred i've addressed your changes

@Wilfred

This comment has been minimized.

Owner

Wilfred commented May 23, 2017

Looks great! There's a compile warning about an unused macro defun_many, which seems to be why the build is failing. You should be able to merge once that's fixed :)

@birkenfeld birkenfeld referenced this pull request May 28, 2017

Closed

Proc macro updates #192

@jeandudey jeandudey merged commit 7609f55 into Wilfred:master May 28, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment