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 char-width #1151

Open
brotzeit opened this issue Dec 19, 2018 · 20 comments · May be fixed by #1583
Open

Port char-width #1151

brotzeit opened this issue Dec 19, 2018 · 20 comments · May be fixed by #1583

Comments

@brotzeit
Copy link
Member

DEFUN ("char-width", Fchar_width, Schar_width, 1, 1, 0,
       doc: /* Return width of CHAR when displayed in the current buffer.
The width is measured by how many columns it occupies on the screen.
Tab is taken to occupy `tab-width' columns.
usage: (char-width CHAR)  */)
  (Lisp_Object ch)
{
  int c;
  ptrdiff_t width;

  CHECK_CHARACTER (ch);
  c = XINT (ch);
  width = char_width (c, buffer_display_table ());
  return make_number (width);
}
@Bolt64
Copy link

Bolt64 commented Dec 21, 2018

I'd like to port this function. Presumably, I'll also need to port the function char_width, but I'm not sure what to do with buffer_display_table, i.e. I'm not sure where it's defined, or whether I can access it from Rust. I'd appreciate a little help.

@shaleh
Copy link
Collaborator

shaleh commented Dec 21, 2018

You can start by calling them both from C.

https://github.com/emacs-mirror/emacs/blob/78ec68e18f07a90a9ad400683b973ff51baa80e1/src/indent.c#L60

All C functions can be imported from remacs_sys.

@agraven agraven added this to To do in Porting efforts via automation Dec 27, 2018
@NQNStudios
Copy link

Would anyone mind if I have a go at this?

@shaleh
Copy link
Collaborator

shaleh commented Apr 18, 2019

A simple port to Rust would call the C functions. Which is why this is labelled as a good first issue. A more interesting port would look at the C functions called and bring them along too.

@NQNStudios
Copy link

I'm looking at the C function buffer_display_table, which references macros BVAR, XCHAR_TABLE, and DISP_TABLE_P. I found the definition for BVAR and DISP_TABLE_P, but not XCHAR_TABLE or at least one of the macros used by DISP_TABLE_P. I'm using rg to try and find original definitions in the C code. Are those macros I'm missing defined in external C libraries?

Trying to delve into the C for a better understanding, but in the meantime I'll go for the simple port option and open a WIP PR.

@shaleh
Copy link
Collaborator

shaleh commented Apr 19, 2019

INLINE struct Lisp_Char_Table *
XCHAR_TABLE (Lisp_Object a)
{
  eassert (CHAR_TABLE_P (a));
  return XUNTAG (a, Lisp_Vectorlike);
}

Defined around line 1818 of lisp.h.

In general XFOO means "hard cast to FOO, do no checks". This is an exception.

@shaleh
Copy link
Collaborator

shaleh commented Apr 19, 2019

BTW, check out https://github.com/Wilfred/deadgrep I have it bound to C-c g.

@NQNStudios
Copy link

So my rg didn't find it because it's not a #define macro. Is it a macro at all? Is INLINE a macro that contains a nested #define? What's a better regex to search for definitions of X___() conversions?

@NQNStudios
Copy link

I guess by using into() I shouldn't necessarily need to understand what types the C functions are using?

@NQNStudios
Copy link

NQNStudios commented Apr 19, 2019

I see from the most recent commit that make_number is no longer a thing? Should I replace the usage of make_number in the C char_width() function with a match val.into() expression like is used here?

If that's the case, is it an idiom that we could DRY somehow, either with a function or a macro?

@brotzeit
Copy link
Member Author

I think we shouldn't use into too much. You can return width as EmacsInt.

@brotzeit
Copy link
Member Author

Probably it's ok if you use into when the return type is EmacsInt. But I don't like it when you can't see the resulting type.

@shaleh
Copy link
Collaborator

shaleh commented Apr 23, 2019

INLINE in C works like the inline directive in Rust. It causes the compiler to replace every instance where the function would be called with the code of the function instead. This reduces oveerhead of building call stacks, jumping in and out of functions, etc. Mentally it behaves like a macro but you have to reason about it as if it is a function.

@shaleh
Copy link
Collaborator

shaleh commented Apr 23, 2019

As for finding the XFOO items look for the define, then look for a line starting with XFOO. That will catch inlines. Or simply look at headers which is all you need to learn about the type.

@shaleh
Copy link
Collaborator

shaleh commented Apr 23, 2019

We consider the into and from usage reasonably DRY. We have been adding force_foo methods lately to mimic the XFOO behavior of not performing checking where it makes sense. Essentially the X routines in C are equivalent to thing.into().unwrap() most of the time.

@shaleh
Copy link
Collaborator

shaleh commented Apr 23, 2019

These are all really good questions @NQNStudios. Thanks for taking the time.

@A6GibKm
Copy link

A6GibKm commented Oct 2, 2019

Is this fine?

use crate::{
   // ...
    remacs_sys::{buffer_display_table, char_width, CHECK_CHARACTER},
};

/// Return width of CHAR when displayed in the current buffer.
/// The width is measured by how many columns it occupies on the screen.
/// Tab is taken to occupy `tab-width' columns.
#[lisp_fn(c_name = "char_width", name = "char-width")]
pub fn char_width_lisp(ch: LispObject) -> EmacsInt {
    CHECK_CHARACTER(ch);
    let c: usize = ch.into().unwrap();
    let width = char_width(c, buffer_display_table());
    width.into().unwrap()
}

EDIT: this is NOT fine.

@denis631
Copy link
Contributor

@A6GibKm why is this not fine? would you like to elaborate?
would be nice to finish this task, seems like you almost got it ;)

@A6GibKm
Copy link

A6GibKm commented Dec 27, 2019

It compiles, but the results I got from the function are not accurate.

@shaleh
Copy link
Collaborator

shaleh commented Jan 6, 2020

@A6GibKm please post a PR, then we can comment on the code as you make progress.

@A6GibKm A6GibKm linked a pull request Aug 24, 2020 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging a pull request may close this issue.

6 participants