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

[Kenichi Handa]

In article <address@hidden>, "Eli Zaretskii" <address@hidden> writes:
>>  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?

emacs-bidi is branched from emacs-unicode-2.

>>  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);

Thank you.  At least, with this change, the immediate crash
can be avoided.

> 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.

I used L2R just to make the test simpler.

>>  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);

I made the "else" part as below.

  else
    {
      bidi_it->bytepos = CHAR_TO_BYTE (pos);
      bidi_it->ch_len
        = MULTIBYTE_FORM_LENGTH (BYTE_POS_ADDR (bidi_it->bytepos),
                                 MAX_MULTIBYTE_LENGTH);
    }

> (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.)

The above MULTIBYTE_FORM_LENGTH does it.

I've just commited these changes.

---
Ken'ichi HANDA
address@hidden