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 fontp to Rust #248

Merged
merged 6 commits into from Jul 24, 2017
Merged

Port fontp to Rust #248

merged 6 commits into from Jul 24, 2017

Conversation

@atheriel
Copy link
Contributor

@atheriel atheriel commented Jul 17, 2017

This is my first pass at porting fontp to Rust, and should be considered a work in progress. I have a few questions:

  1. Should I change the naming convention for the font_property_index enum?
  2. Is there a convention for calling intern from Rust code? See #255.
  3. Can or should I add is_font_spec() (etc) methods to LispObject in lisp.rs, so that the checking can be moved out of fontp and into those methods?

This should close #233.

// See font_property_index in font.h for details.
#[allow(non_camel_case_types, dead_code)]
#[repr(C)]
enum font_property_index {

This comment has been minimized.

@DavidDeSimone

DavidDeSimone Jul 17, 2017
Collaborator

This enum will probably want to be defined in the remacs-sys crate instead of here. That crate is where we house the code related to FFI and representing structures that are defined in the C layer.

This comment has been minimized.

@atheriel

atheriel Jul 17, 2017
Author Contributor

Ok, I will move it there.

This comment has been minimized.

@shanavas786

shanavas786 Jul 18, 2017
Collaborator

use camelcase names.

This comment has been minimized.

@atheriel

atheriel Jul 19, 2017
Author Contributor

Moved, with a new camelcase name.

if v.is_pseudovector(PseudovecType::PVEC_FONT) {
if extra_type.eq(Qnil) {
LispObject::constant_t()
} else if extra_type.eq(LispObject::from_raw(unsafe { Qfont_spec })) {

This comment has been minimized.

@DavidDeSimone

DavidDeSimone Jul 17, 2017
Collaborator

Yeah I don't like the raw unsafe Q... usage here. Personally I like the idea of having safe methods implemented for LispObject like "is_font_spec", etc.

This comment has been minimized.

@atheriel

atheriel Jul 17, 2017
Author Contributor

I thoought it would be "safe enough" to get at these symbols this way -- is there another option?

This comment has been minimized.

@DavidDeSimone

DavidDeSimone Jul 17, 2017
Collaborator

Let me clarify:

Accessing these symbols the way you are doing is fine, but we have to use unsafe blocks in Lisp binding code. If we have another lisp binding that needs to do similar logic, we will need to add more unsafe blocks. In general, I think a Rust code base is at it's best when it minimizes unsafe code blocks.

We can improve this situation by adding a safe wrapper around unsafe code (since the code here is "unsafe" due to its interaction with C). For example something like:

impl LispObject {
    pub fn is_font_spec(&self) -> bool {
        self.eq(unsafe { /* ... */ });  
    }
}

This comment has been minimized.

@atheriel

atheriel Jul 18, 2017
Author Contributor

That's fair. I think that fontp is the only place that uses of Qfont_spec and friends in this manner in the codebase, though. Is it worth writing a safe interface (for example, creating a FontEntity::from_object()-style enum method)?

This comment has been minimized.

@DavidDeSimone

DavidDeSimone Jul 18, 2017
Collaborator

That's a fair point, I'm personally fine either way.

This comment has been minimized.

@atheriel

atheriel Jul 19, 2017
Author Contributor

I wrote a little enum to abstract this pattern out.

} else {
// As with the C version, checking that object is a font takes priority
// over checking that extra_type is well-formed.
Qnil

This comment has been minimized.

@DavidDeSimone

DavidDeSimone Jul 17, 2017
Collaborator

We generally prefer calls to LispObject::constant_nil() over raw Qnil usage.

This comment has been minimized.

@atheriel

atheriel Jul 19, 2017
Author Contributor

Changed.

@DavidDeSimone
Copy link
Collaborator

@DavidDeSimone DavidDeSimone commented Jul 17, 2017

Thanks for the contribution! This is looking good to me so far, I've left a few inline comments on a couple things I saw.

@atheriel
Copy link
Contributor Author

@atheriel atheriel commented Jul 18, 2017

Ok, I've added some is_font_* methods to LispObject.

@atheriel
Copy link
Contributor Author

@atheriel atheriel commented Jul 18, 2017

I will probably wait on #244 before moving the struct to remacs-sys.

#[lisp_fn(min = "1")]
pub fn fontp(object: LispObject, extra_type: LispObject) -> LispObject {
if object.is_font() {
if extra_type.eq(LispObject::constant_nil()) {

This comment has been minimized.

@birkenfeld

birkenfeld Jul 18, 2017
Collaborator

shorter: extra_type.is_nil()

This comment has been minimized.

@atheriel

atheriel Jul 19, 2017
Author Contributor

Changed.

@@ -559,6 +560,27 @@ impl LispObject {
v.is_pseudovector(PseudovecType::PVEC_FONT)
})
}

pub fn is_font_spec(self) -> bool {

This comment has been minimized.

@birkenfeld

birkenfeld Jul 18, 2017
Collaborator

Instead of adding these to LispObject, I would create an as_font method on is_vectorlike that returns a "LispFontRef" or similar.

is_spec, is_entity etc can the be methods on that struct.

This comment has been minimized.

@atheriel

atheriel Jul 19, 2017
Author Contributor

Ok, I've taken this approach.

@atheriel atheriel force-pushed the atheriel:port-fontp branch from 4f920a2 to fe82e73 Jul 19, 2017
}

impl FontExtraType {
pub fn from_object_or_error(extra_type: LispObject) -> FontExtraType {

This comment has been minimized.

@birkenfeld

birkenfeld Jul 19, 2017
Collaborator

I'd prefer from_symbol_or_error.

This comment has been minimized.

@atheriel

atheriel Jul 20, 2017
Author Contributor

Done.

FontExtraType::Object
} else {
// TODO: This should actually be equivalent to
// intern("font-extra-type"), not Qsymbolp.

This comment has been minimized.

@birkenfeld

birkenfeld Jul 19, 2017
Collaborator

In order to call intern, you can add it to the imported functions in remacs-sys. Signature should be straightforward.

This comment has been minimized.

@atheriel

atheriel Jul 20, 2017
Author Contributor

This turns out to be more difficult than it first appears. See #255.

This comment has been minimized.

@atheriel

atheriel Jul 23, 2017
Author Contributor

Done.

@@ -1096,3 +1096,47 @@ extern "C" {
nchars_return: *mut ptrdiff_t,
) -> ptrdiff_t;
}

/// Contains C definitions from the font.h header.
pub mod font {

This comment has been minimized.

@birkenfeld

birkenfeld Jul 19, 2017
Collaborator

Good idea to add some modules to structure this crate!

@atheriel atheriel force-pushed the atheriel:port-fontp branch from fe82e73 to fd203a3 Jul 20, 2017
@birkenfeld
Copy link
Collaborator

@birkenfeld birkenfeld commented Jul 23, 2017

Can be updated to use intern() now.

atheriel added 6 commits Jul 16, 2017
Signed-off-by: Aaron Jacobs <atheriel@gmail.com>
Signed-off-by: Aaron Jacobs <atheriel@gmail.com>
Signed-off-by: Aaron Jacobs <atheriel@gmail.com>
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 atheriel force-pushed the atheriel:port-fontp branch from fd203a3 to 728579b Jul 23, 2017
@atheriel
Copy link
Contributor Author

@atheriel atheriel commented Jul 23, 2017

Updated to use intern(). Is there anything else extant on this PR?

@birkenfeld
Copy link
Collaborator

@birkenfeld birkenfeld commented Jul 24, 2017

Nope, looks, good!

@birkenfeld birkenfeld merged commit 7016d43 into remacs:master Jul 24, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
use vectors::LispVectorlikeRef;

// A font is not a type in and of itself, it's just a group of three kinds of
// pseudovector. This newtype allows us to define methods that yield the actual

This comment has been minimized.

@thisch

thisch Jul 24, 2017

This could be improved.

  • plural of pseudovectors
  • missing space between new and type.
  • First you mention that a font is not a type and then you continue with "This new type"

This comment has been minimized.

@atheriel

atheriel Jul 26, 2017
Author Contributor

I'm glad someone is reading the docstrings :) Not quite sure I agree about the plural -- a font is "one of three kinds of pseudovector", so fonts are "a group of three kinds of pseudovector". Might be wrong here, though.

The term "newtype" refers to the Rust pattern of struct X(Y). It's not a typo. A "font" does not have its own structure in the C code. It refers to three possible structs. Hence a font is not "a type in and of itself". It's just a label for a group of structures. So I'm not actually defining any structure here, I just want to add some methods to LispVectorlikeRef -- hence the newtype.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants
You can’t perform that action at this time.