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 bolp and eolp to rust #293

Merged
merged 11 commits into from Sep 2, 2017
Merged

Port bolp and eolp to rust #293

merged 11 commits into from Sep 2, 2017

Conversation

kidd
Copy link
Member

@kidd kidd commented Aug 16, 2017

With the PR as it is, I get ^^^^^^^^^^ expected u8, found char, which makes sense because I'm comparing the address offset address with the char '\n' itself.

I couldn't find the function that would get the char at a given addr (something with a signature like: buffer.char_at_addr(addr)). Do we have such function?

Also, is that use of unsafe, safe? :) or at least, sensible?

@Wilfred
Copy link
Collaborator

Wilfred commented Aug 16, 2017

I think the use of unsafe here is reasonable. I believe .text is never NULL (surely a buffer always has text?).

We don't have an API for accessing bytes in buffers yet. Looking at the relevant C macros:

/* Return the byte at byte position N.  */

#define FETCH_BYTE(n) *(BYTE_POS_ADDR ((n)))

#define BYTE_POS_ADDR(n) \
  (((n) >= GPT_BYTE ? GAP_SIZE : 0) + (n) + BEG_ADDR - BEG_BYTE)

This looks like it's doing pointer arithmetic of bytes before or after the gap. We should probably do the same thing.

@kidd kidd changed the title WIP: Port bolp to rust WIP: Port bolp and eolp to rust Aug 17, 2017
@kidd
Copy link
Member Author

kidd commented Aug 17, 2017

I think now it's in a mergeable state (if travis says 'green'). There was some trial and error in the castings (casting everything to u32, then to isize, and ended up using ptrdiff_t (I'd appreciate any feedback)).

Also ported eolp.

There's clearly some space for refactoring in both (they are basically (bobp() || .....) and (eobp() || ....) and we're not reusing them, but I'm not confident enough we want to create new internal functions with different signatures (bool instead of LispObject).

@kidd kidd changed the title WIP: Port bolp and eolp to rust Port bolp and eolp to rust Aug 17, 2017
@kidd
Copy link
Member Author

kidd commented Aug 17, 2017

mmm... It looks like it's a formatting issue.

The thing is that if I run [~/remacs/rust_src] $ cargo fmt -- --write-mode=diff I get lots of other diffs.

Any hints? (of course, it's my first time running cargo fmt)

Thanks!

@birkenfeld
Copy link
Collaborator

You might have an old version of rustfmt. The default style changed quite a lot with 0.9.0.

Copy link
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 a few minor things...

0
};
let byte_addr = (base_addr + n +
self.beg_addr() as ptrdiff_t - self.beg_byte()) as *const u8;
Copy link
Collaborator

Choose a reason for hiding this comment

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

looks like that should be self.beg_addr().offset(base_addr + n - self.beg_byte())

#[lisp_fn]
pub fn bolp() -> LispObject {
let buffer_ref = ThreadState::current_buffer();
let pt_byte = buffer_ref.pt_byte - 1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm, calling pt_byte - 1 also pt_byte is a bit confusing.

let buffer_ref = ThreadState::current_buffer();
let pt_byte = buffer_ref.pt_byte - 1;
LispObject::from_bool(buffer_ref.pt == buffer_ref.begv ||
buffer_ref.fetch_byte(pt_byte) as char == '\n')
Copy link
Collaborator

Choose a reason for hiding this comment

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

instead of as char, compare with b'\n'

@kidd
Copy link
Member Author

kidd commented Aug 17, 2017

Thanks for the comments! I'll fix those asap.

* Formatted according to rust fmt
* Compare bytes using b''
Copy link
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.

One tiny comment, but this is nice Rusty code :)

I'm not sure why the build has failed though. It looks like a timeout. It's annoying if so (it means our build is unreliable) but I don't see how these changes could relate.


#[inline]
pub fn fetch_byte(&self, n: ptrdiff_t) -> u8 {
let base_addr = if n >= self.gpt_byte() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd prefer we called this offset -- it's not actually an address (0 wouldn't make sense as an address here).

pub fn eolp() -> LispObject {
let buffer_ref = ThreadState::current_buffer();
LispObject::from_bool(
buffer_ref.pt == buffer_ref.zv() || buffer_ref.fetch_byte(buffer_ref.pt_byte) == b'\n',
Copy link
Member Author

Choose a reason for hiding this comment

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

cargo fmt told me about that final comma, but.... does it make sense? I find it a bit strange, no?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed, it's a little odd. Perhaps it's a rustfmt bug.

I like that rustfmt is a tool that you just run and it fixes formatting without human input. We could file a bug upstream, but I'd leave the comma for now if rustfmt wants it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think it's a big, it's just consistent in always having a final comma on multi-line lists of expressions. The argument for having the trailing comma in the first place, less churn when an element is added, applies to single arguments as well.

@kidd
Copy link
Member Author

kidd commented Aug 24, 2017

still timeouts :(. Next week I'm going to have some time to bisect over the commits, or try to investigate a bit more. It's a pity it doesn't happen in my local machine, just in travisCI :(

@Wilfred
Copy link
Collaborator

Wilfred commented Aug 29, 2017

@kidd have you tried running the test suite yourself? Does it terminate? I've been thinking that (while (not (bolp)) ...) is quite common, so it's possible a bug has made a test go into an infinite loop with this PR.

@kidd
Copy link
Member Author

kidd commented Aug 31, 2017

@Wilfred you're right, the infinite loop was caused by not returning eolp => t in certain situations.

The bug was introduced during the latest refactors and went under the radar. Hopefully now it's green.
EDIT: nope. looking into it, the error is in: https://travis-ci.org/Wilfred/remacs/jobs/270489438#L1560 . I don't know how I got anything to do with this warning appearing now.

@kidd
Copy link
Member Author

kidd commented Sep 2, 2017

@Wilfred Wilfred dismissed birkenfeld’s stale review September 2, 2017 20:40

I believe all the issues mentioned have been resolved :)

@Wilfred Wilfred merged commit 15695bc into remacs:master Sep 2, 2017
@Wilfred
Copy link
Collaborator

Wilfred commented Sep 2, 2017

Awesome :)

@kidd kidd deleted the port-bolp branch September 3, 2017 08:49
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.

None yet

3 participants