Re: [emacs-bidi] branch emacs-bidi is created

[Eli Zaretskii]

> Date: Thu, 4 Mar 2004 09:07:22 +0900 (JST)
> From: Kenichi Handa <address@hidden>
> 
> I've just made a branch "emacs-bidi" and commited several
> changes including Eli's bidi code so that people other than
> I can start working on it.

Does this branch use the emacs-unicode as its base, or did you start
from CVS HEAD?

> But, although I can compile the code and start up Emacs,
> when I set the buffer local variable enable-bidi-display,
> Emacs crashes.   Eli, I need your help.

I didn't have time to check out the bidi branch, so the following is
based on code inspection.

> In the reseat_1 (in xdisp.c), I put this code.
> 
>   if (it->bidi_p)
>     {
>       bidi_init_it (pos.charpos, L2R, &it->bidi_it);
>       bidi_get_next_char_visually (&it->bidi_it);
> 
>       pos.charpos = it->bidi_it.charpos;
>       pos.bytepos = it->bidi_it.bytepos;
>       it->current.pos = it->position = pos;
>     }
> 
> But, as bidi_init_it doesn't set bidi_it->ch_len,
> bidi_get_next_char_visually sets it->bidi_it.bytepos to less
> than it->bidi_it.charpos.

Hmm, I knew this initialization thing will come back to haunt
me...  It's a bit tricky: the bidi iterator increments the position
(inside bidi_get_next_char_visually) _before_ it examines the next
character, so it needs to be initialized at the position one less than
where you want it to begin its iteration.  So I think you want to
change

>       bidi_init_it (pos.charpos, L2R, &it->bidi_it);

into

>       bidi_init_it (pos.charpos - 1, L2R, &it->bidi_it);

Btw, why did you use L2R as the second argument?  I think we should
use NEUTRAL_DIR instead, so that the code finds the paragraph
direction from the first strong directional character, as mandated by
UAX#9.

> bidi_init_it has this code:
> 
> bidi_init_it (int pos, bidi_dir_t dir, struct bidi_it *bidi_it)
> {
>   if (! bidi_initialized)
>     bidi_initialize ();
>   bidi_set_paragraph_end (bidi_it);
>   bidi_it->charpos = pos;
>   if (pos <= 0)
>     {
>       bidi_it->bytepos = bidi_it->charpos;
>       bidi_it->ch_len = 1;    /* so that incrementing bytepos works */
>     }
>   else
>     bidi_it->bytepos = CHAR_TO_BYTE (pos);
> 
> As POS is always greater than 0 when we are scanning a
> buffer, bidi_it->ch_len is not set.

I'm guessing that the else branch was never executed in my testing.
For completeness, I think setting bidi_it->ch_len in the else clause
like below will do:

      bidi_it->ch_len = CHAR_TO_BYTE(pos+1) - CHAR_TO_BYTE(pos);

In case this is too expensive, we could use the paradigm used by
xdisp.c to compute it->len:

      if (it->multibyte_p && !ASCII_BYTE_P (*p))
        {
          int maxlen = ((IT_BYTEPOS (*it) >= GPT_BYTE ? ZV_BYTE : GPT_BYTE)
                        - IT_BYTEPOS (*it));
          int c = string_char_and_length (p, maxlen, &it->len);
        }
      else
        it->len = 1;

(Btw, I think we sorely need an efficient macro for computing the
length, and only the length, of a multibyte character at a given
position in the buffer.)

Thanks again for making this happen.