Skip to content

Add LispObarrayRef and port intern functions to Rust#267

Merged
birkenfeld merged 8 commits intoremacs:masterfrom
atheriel:obarray
Aug 7, 2017
Merged

Add LispObarrayRef and port intern functions to Rust#267
birkenfeld merged 8 commits intoremacs:masterfrom
atheriel:obarray

Conversation

@atheriel
Copy link
Copy Markdown
Contributor

This PR ports a chunk of the obarray-related code from lread.c to Rust, including the lisp functions intern and intern-soft. I had to add some extern functions to the C code in order to get access to some global state.

Comment thread rust_src/src/lisp.rs Outdated
}

/// Intern (e.g. create a symbol from) a string.
#[allow(dead_code)]
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

dead_code still needed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Nope. Fixed.

Comment thread rust_src/src/obarray.rs Outdated
/// This is a wrapper around the C function `oblookup`. It is capable of
/// handling multibyte strings, unlike intern_1 and friends.
pub fn lookup(&self, name: LispObject) -> LispObject {
let string = if name.is_symbol() {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It's still better to use as_symbol which does the conversion already.

Anyway, this is basically LispObject::symbol_or_string_as_string.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Comment thread src/lread.c
return obarray;
}

/* Like check_obarray, but for the global Vobarray. */
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is it not possible to access the V... from Rust?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No, it's not exposed in any .h files. I got a linker error when I tried. Is there another way?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

it looks like Vobarray is defined as #define Vobarray globals.f_Vobarray in globals.h. We set up the emacs globals as remacs_sys::globals, so it seems like if we need access to the V... variables in Rust, we can just use the global object (unless I am misunderstanding the situation).

Copy link
Copy Markdown
Contributor Author

@atheriel atheriel Jul 27, 2017

Choose a reason for hiding this comment

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

So... at first I thought this would work. But if I replace

let obarray = unsafe { check_vobarray() };

with

let obarray = unsafe { check_obarray(globals.f_Vobarray) };

The bootstrapping process fails on the first call to intern_1 in the C code, and Emacs will not build past the temacs stage, because globals.f_Vobarray is inexplicably nil at that point. This is a totally bizarre bug I haven't been able to track down. My best guess at the moment is that the globals object might have different contents for obarray during the bootstrap process and the regular Emacs incarnation.

Any thoughts on this would be appreciated.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ouch, that's odd. Let's open an issue and fix that, but I don't think it should hold up this PR.

Copy link
Copy Markdown
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.

I like your LispObarrayRef, looks good :)

Generally looks good to me, but I agree we should use globals.f_Vobarray. Other than that, I think this is good to go ✨

Copy link
Copy Markdown
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.

Sorry for the delay on this.

I think this is ready to merge. It would be nice to fix the globals.V_obarray issue, but that shouldn't delay this PR. It's nice clean Rust and makes it easier for us to build additional symbol functionality.

@atheriel
Copy link
Copy Markdown
Contributor Author

atheriel commented Aug 2, 2017

@Wilfred I've actually been working on other major symbol table work that builds on this PR, and I fixed one of the globals issues in the process. Let me upstream that fix and rebase tonight, and then this first part can be merged.

In order to get access to the `obarray` lisp object, I've had to add
an extern function to the C sources: `check_vobarray`.

Signed-off-by: Aaron Jacobs <atheriel@gmail.com>
Signed-off-by: Aaron Jacobs <atheriel@gmail.com>
This commit adds an extern function to the C code in order to get
access to a previously un-exposed piece of global state: whether Emacs
has set the `purify-flag'.

Signed-off-by: Aaron Jacobs <atheriel@gmail.com>
Signed-off-by: Aaron Jacobs <atheriel@gmail.com>
Signed-off-by: Aaron Jacobs <atheriel@gmail.com>
@atheriel
Copy link
Copy Markdown
Contributor Author

atheriel commented Aug 3, 2017

This should be good to go now.

Copy link
Copy Markdown
Collaborator

@birkenfeld birkenfeld left a comment

Choose a reason for hiding this comment

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

Just some minor comments.

Comment thread rust_src/src/lib.rs
pub use lists::merge;
pub use buffers::Fget_buffer;
pub use buffers::Fcurrent_buffer;
pub use obarray::intern_1;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

You're declaring intern_1 in remacs-sys but also exporting it here...

Comment thread rust_src/src/obarray.rs Outdated
/// This is a wrapper around the C function `oblookup`. It is capable of
/// handling multibyte strings, unlike intern_1 and friends.
pub fn lookup(&self, name: LispObject) -> LispObject {
let string = LispObject::symbol_or_string_as_string(name);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Should use normal method call syntax here.

@birkenfeld
Copy link
Copy Markdown
Collaborator

Resolved conflicts and my two suggestions.

@birkenfeld birkenfeld merged commit 11bfbcf into remacs:master Aug 7, 2017
@atheriel
Copy link
Copy Markdown
Contributor Author

atheriel commented Aug 7, 2017

@birkenfeld Thanks for doing that, I wasn't able to get back to it this past weekend.

@atheriel atheriel deleted the obarray branch August 7, 2017 16:35
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.

5 participants