1:04 a.m.
separate_textual_runs was a nice, simple function ages ago, but not
anymore. Now it's a not very readable ifdef mess. Building 4
functions for the mule/nomule, xft/xlib cases allows for drastic
simplifications for most of them.
Please review (it's hairy code, another pair of eyes is no luxury) and
apply if happy. It's probably easier in that specific case to check
the result rather than the diff.
OG.
2007-03-30 Olivier Galibert <galibert(a)pobox.com>
* redisplay-x.c (separate_textual_runs_xft_mule): Create.
(separate_textual_runs_xft_nomule): Create.
(separate_textual_runs_mule): Create.
(separate_textual_runs_nomule): Create.
(separate_textual_runs): Split the ifdef mess into 4 separate
functions and simplify each.
_______________________________________________
XEmacs-Patches mailing list
XEmacs-Patches(a)xemacs.org
http://calypso.tux.org/cgi-bin/mailman/listinfo/xemacs-patches
5:51 a.m.
Olivier Galibert writes:
Please review (it's hairy code, another pair of eyes is no
luxury) and
apply if happy. It's probably easier in that specific case to check
the result rather than the diff.
A careful review later, but first a cosmetic suggestion: reorder the
functions to go from simpler to more complex, probably
separate_textual_runs_nomule
separate_textual_runs_xft_nomule
separate_textual_runs_xft_mule
separate_textual_runs_mule
+ for (i = 0; i < len; i++)
+ *text_storage++ = str[i];
Did the no-Mule, no-Xft case get *too bald* for you so you had to
introduce some hair?<wink>
_______________________________________________
XEmacs-Patches mailing list
XEmacs-Patches(a)xemacs.org
http://calypso.tux.org/cgi-bin/mailman/listinfo/xemacs-patches
7:45 a.m.
New subject: [Simplify redisplay-x/separate_textual_runs] Fix union build, shut up gcc
Olivier Galibert writes:
+separate_textual_runs_xft_mule (unsigned char *text_storage,
+ struct textual_run *run_storage,
+ const Ichar *str, Charcount len,
+ struct face_cachel *cachel)
Except for non-Xft Mule, `cachel' should be declared UNUSED, it
seems. (This shuts up a warning, it's not an error.)
+ Lisp_Object charset = ichar_charset(ch);
[...]
+ if (prev_charset != charset)
Tsk, tsk. EQ is your friend here.<wink>
Builds, passes "make check" (ie, only the KNOWN BUGs fail), and runs
(not used extensively yet). Interestingly, it doesn't show the
missing line/column info bug reported yesterday.
steve＠uwakimon /playpen/src/XEmacs/git-integration $ steve＠uwakimon
/playpen/src/XEmacs/git-integration $ git diff
diff --git a/src/redisplay-x.c b/src/redisplay-x.c
index 4b49d72..d2f8e29 100644
--- a/src/redisplay-x.c
+++ b/src/redisplay-x.c
＠＠ -149,7 +149,7 ＠＠ static int
separate_textual_runs_xft_mule (unsigned char *text_storage,
struct textual_run *run_storage,
const Ichar *str, Charcount len,
- struct face_cachel *cachel)
+ struct face_cachel * UNUSED(cachel))
{
Lisp_Object prev_charset = Qnil;
int runs_so_far = 0, i;
＠＠ -170,7 +170,7 ＠＠ separate_textual_runs_xft_mule (unsigned char *text_storage,
/* That means we can't handle characters outside of the BMP for now */
ucs = (ucs & ~0xFFFF) ? 0xFFFD : ucs;
- if (prev_charset != charset)
+ if (!EQ (prev_charset, charset))
{
if (runs_so_far)
run_storage[runs_so_far-1].len = (text_storage - run_storage[runs_so_far-1].ptr)
>> 1;
＠＠ -196,7 +196,7 ＠＠ static int
separate_textual_runs_xft_nomule (unsigned char *text_storage,
struct textual_run *run_storage,
const Ichar *str, Charcount len,
- struct face_cachel *cachel)
+ struct face_cachel * UNUSED(cachel))
{
int i;
if (!len)
＠＠ -348,7 +348,7 ＠＠ static int
separate_textual_runs_nomule (unsigned char *text_storage,
struct textual_run *run_storage,
const Ichar *str, Charcount len,
- struct face_cachel *cachel)
+ struct face_cachel * UNUSED(cachel))
{
int i;
if (!len)
_______________________________________________
XEmacs-Patches mailing list
XEmacs-Patches(a)xemacs.org
http://calypso.tux.org/cgi-bin/mailman/listinfo/xemacs-patches
7:49 a.m.
The patch certainly reads well---thanks!
I'll wait a few hours to see if Stephen's comment makes you supersede
your patch, and then I'll try it out.
--
Cheers =8-} Mike
Friede, Völkerverständigung und überhaupt blabla
_______________________________________________
XEmacs-Patches mailing list
XEmacs-Patches(a)xemacs.org
http://calypso.tux.org/cgi-bin/mailman/listinfo/xemacs-patches
10:45 a.m.
On Fri, Mar 30, 2007 at 01:51:51PM +0900, Stephen J. Turnbull wrote:
Olivier Galibert writes:
> Please review (it's hairy code, another pair of eyes is no luxury) and
> apply if happy. It's probably easier in that specific case to check
> the result rather than the diff.
A careful review later, but first a cosmetic suggestion: reorder the
functions to go from simpler to more complex, probably
separate_textual_runs_nomule
separate_textual_runs_xft_nomule
separate_textual_runs_xft_mule
separate_textual_runs_mule
No problem. I frankly don't care either way :-)
> + for (i = 0; i < len; i++)
> + *text_storage++ = str[i];
Did the no-Mule, no-Xft case get *too bald* for you so you had to
introduce some hair?<wink>
Hmmm. More of a case of not removing enough. What would you like,
memcpy? Reuse str? (I'm not sure about the relative lifetimes of
run_storage, text_storage and str...).
OG.
_______________________________________________
XEmacs-Patches mailing list
XEmacs-Patches(a)xemacs.org
http://calypso.tux.org/cgi-bin/mailman/listinfo/xemacs-patches
11:01 a.m.
New subject: [Simplify redisplay-x/separate_textual_runs] Fix union build, shut up gcc
On Fri, Mar 30, 2007 at 03:45:31PM +0900, Stephen J. Turnbull wrote:
Olivier Galibert writes:
> +separate_textual_runs_xft_mule (unsigned char *text_storage,
> + struct textual_run *run_storage,
> + const Ichar *str, Charcount len,
> + struct face_cachel *cachel)
Except for non-Xft Mule, `cachel' should be declared UNUSED, it
seems. (This shuts up a warning, it's not an error.)
Didn't know about this syntax. Should have looked.
> + Lisp_Object charset = ichar_charset(ch);
[...]
> + if (prev_charset != charset)
Tsk, tsk. EQ is your friend here.<wink>
I guess I have something against EQ. No idea why...
Builds, passes "make check" (ie, only the KNOWN BUGs fail),
and runs
(not used extensively yet). Interestingly, it doesn't show the
missing line/column info bug reported yesterday.
Neat. Simplifications tend to help, in practice.
Once you've answered about the str/memcpypart, I'll do a new, probably
final patch integrating your remarks.
The only part I'm really doubtful about is the ucs->byte split in the
-xft+mule translate_to_ics2 case. I don't even know how to have the
code pass through there.
OG.
_______________________________________________
XEmacs-Patches mailing list
XEmacs-Patches(a)xemacs.org
http://calypso.tux.org/cgi-bin/mailman/listinfo/xemacs-patches
3:49 p.m.
Ar an triochadú lá de mí Márta, scríobh Olivier Galibert:
separate_textual_runs was a nice, simple function ages ago, but not
anymore. Now it's a not very readable ifdef mess. Building 4
functions for the mule/nomule, xft/xlib cases allows for drastic
simplifications for most of them.
Please review (it's hairy code, another pair of eyes is no luxury) and
apply if happy. It's probably easier in that specific case to check
the result rather than the diff.
[...]
＠＠ -297,11 +335,54 ＠＠
run_storage[runs_so_far - 1].len =
text_storage - run_storage[runs_so_far - 1].ptr;
/* Dimension retains the relevant value for the run before it. */
- if (2 == dimension)
+ if (dimension == 2)
run_storage[runs_so_far - 1].len >>= 1;
}
For C, the CONSTANT == VARIABLE idiom has a clear advantage over the other
way around (that is, ‘2 = dimension’ doesn’t compile) and the more it’s
used, the more readable it gets. Please don’t change this idiom in future.
--
On the quay of the little Black Sea port, where the rescued pair came once
more into contact with civilization, Dobrinton was bitten by a dog which was
assumed to be mad, though it may only have been indiscriminating. (Saki)
_______________________________________________
XEmacs-Patches mailing list
XEmacs-Patches(a)xemacs.org
http://calypso.tux.org/cgi-bin/mailman/listinfo/xemacs-patches
4:14 p.m.
Ar an triochadú lá de mí Márta, scríobh Olivier Galibert:
+#if defined(USE_XFT) && defined(MULE)
static int
-separate_textual_runs (unsigned char *text_storage,
- struct textual_run *run_storage,
- const Ichar *str, Charcount len,
- struct face_cachel *cachel)
+separate_textual_runs_xft_mule (unsigned char *text_storage,
+ struct textual_run *run_storage,
+ const Ichar *str, Charcount len,
+ struct face_cachel *cachel)
+{
+ Lisp_Object prev_charset = Qnil;
+ int runs_so_far = 0, i;
+
+ run_storage[0].ptr = text_storage;
+ run_storage[0].len = len;
+ run_storage[0].dimension = 2;
+ run_storage[0].charset = Qnil;
+
+ for (i = 0; i < len; i++)
+ {
+ Ichar ch = str[i];
+ Lisp_Object charset = ichar_charset(ch);
+ int ucs = ichar_to_unicode(ch);
+
+ /* If UCS is less than zero or greater than 0xFFFF, set ucs2 to
+ REPLACMENT CHARACTER. */
+ /* That means we can't handle characters outside of the BMP for now */
+ ucs = (ucs & ~0xFFFF) ? 0xFFFD : ucs;
You’ve just done that Unicode conversion to throw away the result. That’s
not very constructive, since the XFT redisplay routines *require* that the
supplied strings are Unicode. I’m fighting with Cygwin right now to get it
to usefully install fontconfig so I can check this out for certain, but I
believe strongly that this will break redisplay with non-Latin-1 characters
under XFT and Mule.
+ if (prev_charset != charset)
+ {
+ if (runs_so_far)
+ run_storage[runs_so_far-1].len = (text_storage - run_storage[runs_so_far-1].ptr)
>> 1;
+ run_storage[runs_so_far].ptr = text_storage;
+ run_storage[runs_so_far].dimension = 2;
+ run_storage[runs_so_far].charset = charset;
+ prev_charset = charset;
+ runs_so_far++;
+ }
+
+ *(XftChar16 *)text_storage = str[i];
+ text_storage += sizeof(XftChar16);
+ }
+
+ if (runs_so_far)
+ run_storage[runs_so_far-1].len = (text_storage - run_storage[runs_so_far-1].ptr)
>> 1;
+ return runs_so_far;
+}
+#endif
--
On the quay of the little Black Sea port, where the rescued pair came once
more into contact with civilization, Dobrinton was bitten by a dog which was
assumed to be mad, though it may only have been indiscriminating. (Saki)
_______________________________________________
XEmacs-Patches mailing list
XEmacs-Patches(a)xemacs.org
http://calypso.tux.org/cgi-bin/mailman/listinfo/xemacs-patches
4:44 p.m.
Ar an triochadú lá de mí Márta, scríobh Aidan Kehoe:
[...] You’ve just done that Unicode conversion to throw away the
result.
That’s not very constructive, since the XFT redisplay routines *require*
that the supplied strings are Unicode. I’m fighting with Cygwin right now
to get it to usefully install fontconfig so I can check this out for
certain, but I believe strongly that this will break redisplay with
non-Latin-1 characters under XFT and Mule.
Confirmed. Cf. http://mid.gmane.org/17780.28830.970989.79622＠parhasard.net
if you’re not sure how to tell XEmacs to use reasonble non-Latin-1 fonts
with XFT. Since that link seems to be broken right now, here’s the code I
use:
(when (featurep 'xft)
(set-specifier
(face-font 'default)
(cons '(x)
(fc-name-unparse
(fc-font-match (selected-device)
(fc-name-parse "Monospace-11.5:lang=en")))))
(loop
for (tag-name regex size) in
'((zh-tw "^chinese-\\(big5\\|cns\\)" 18)
(zh "^chinese-" 18)
(ja "^\\(japanese\\|katakana\\)-" 18)
(ko "^korean-" 18)
(am "^ethiopic-" 18)
(vi "^vietnamese-" 14)
(el "^greek-" 12)
(ru "^cyrillic-" 12)
(hu "^latin-iso8859-2" 12)
(ar "^arabic-" 16))
do
(define-specifier-tag tag-name nil
(list 'lambda '(charset)
(list 'string-match regex
'(symbol-name (charset-name charset)))))
(set-face-font 'default
(fc-name-unparse
(fc-font-match
(selected-device)
(fc-name-parse
(format "-%d:lang=%s"
size tag-name))))
nil (list 'x tag-name) 'append)))
--
On the quay of the little Black Sea port, where the rescued pair came once
more into contact with civilization, Dobrinton was bitten by a dog which was
assumed to be mad, though it may only have been indiscriminating. (Saki)
_______________________________________________
XEmacs-Patches mailing list
XEmacs-Patches(a)xemacs.org
http://calypso.tux.org/cgi-bin/mailman/listinfo/xemacs-patches
6:03 p.m.
On Fri, Mar 30, 2007 at 05:44:38PM +0200, Aidan Kehoe wrote:
Ar an triochadú lá de mí Márta, scríobh Aidan Kehoe:
> [...] You???ve just done that Unicode conversion to throw away the result.
> That???s not very constructive, since the XFT redisplay routines *require*
> that the supplied strings are Unicode. I???m fighting with Cygwin right now
> to get it to usefully install fontconfig so I can check this out for
> certain, but I believe strongly that this will break redisplay with
> non-Latin-1 characters under XFT and Mule.
Confirmed. Cf. http://mid.gmane.org/17780.28830.970989.79622＠parhasard.net
if you???re not sure how to tell XEmacs to use reasonble non-Latin-1 fonts
with XFT. Since that link seems to be broken right now, here???s the code I
use:
Thanks, I'll try.
OG.
_______________________________________________
XEmacs-Patches mailing list
XEmacs-Patches(a)xemacs.org
http://calypso.tux.org/cgi-bin/mailman/listinfo/xemacs-patches
6:58 p.m.
Olivier Galibert writes:
Hmmm. More of a case of not removing enough. What would you like,
memcpy? Reuse str? (I'm not sure about the relative lifetimes of
run_storage, text_storage and str...).
memcpy. Or at least "while (len--) *text_storage++ = *str++;". :-)
_______________________________________________
XEmacs-Patches mailing list
XEmacs-Patches(a)xemacs.org
http://calypso.tux.org/cgi-bin/mailman/listinfo/xemacs-patches
7:48 p.m.
New subject: [Simplify redisplay-x/separate_textual_runs] Fix union build, shut up gcc
Olivier Galibert writes:
The only part I'm really doubtful about is the ucs->byte split
in the
-xft+mule translate_to_ics2 case. I don't even know how to have the
code pass through there.
I think Aidan is the expert on that; I think that stuff is from his
JIT charsets work. I took a hack at explaining it in my second post
(with the header comments, especially for -xft+mule), but I'd want
Aidan's review on that.
_______________________________________________
XEmacs-Patches mailing list
XEmacs-Patches(a)xemacs.org
http://calypso.tux.org/cgi-bin/mailman/listinfo/xemacs-patches
12:13 p.m.
New subject: [Simplify redisplay-x/separate_textual_runs] Fix union build, shut up gcc
Olivier Galibert writes:
Didn't know about UNUSED syntax. Should have looked.
Oh, yeah, you weren't around much. Last year Jerry spent the summer
visiting someplace they were doing research on static checkers, so we
got the benefit of being the guinea pig. Since it was Jerry's
research, he had to write the patches to make sure the checker knew
what it was doing. Good deal, huh?
Anyway, I *think* that UNUSED (and a couple of "USED_IF_condition"
variants, see compiler.h) was the main contribution to syntax from the
static checkers.
_______________________________________________
XEmacs-Patches mailing list
XEmacs-Patches(a)xemacs.org
http://calypso.tux.org/cgi-bin/mailman/listinfo/xemacs-patches
1:50 p.m.
Ar an triochadú lá de mí Márta, scríobh Aidan Kehoe:
Ar an triochadú lá de mí Márta, scríobh Aidan Kehoe:
> [...] You’ve just done that Unicode conversion to throw away the result.
> That’s not very constructive, since the XFT redisplay routines *require*
> that the supplied strings are Unicode. I’m fighting with Cygwin right now
> to get it to usefully install fontconfig so I can check this out for
> certain, but I believe strongly that this will break redisplay with
> non-Latin-1 characters under XFT and Mule.
Confirmed. Cf. http://mid.gmane.org/17780.28830.970989.79622＠parhasard.net
if you’re not sure how to tell XEmacs to use reasonble non-Latin-1 fonts
with XFT. Since that link seems to be broken right now, here’s the code I
use:
(when (featurep 'xft)
That should have been
(when (featurep 'xft-fonts)
by the way. Hopefully you noticed the code’s lack of effect and worked it
out already, but in the event that anyone else is reading, it’s worth making
clear.
--
On the quay of the little Black Sea port, where the rescued pair came once
more into contact with civilization, Dobrinton was bitten by a dog which was
assumed to be mad, though it may only have been indiscriminating. (Saki)
_______________________________________________
XEmacs-Patches mailing list
XEmacs-Patches(a)xemacs.org
http://calypso.tux.org/cgi-bin/mailman/listinfo/xemacs-patches
3:29 p.m.
New subject: [Simplify redisplay-x/separate_textual_runs] Fix union build, shut up gcc
Ar an t-aonú lá is triochad de mí Márta, scríobh Stephen J. Turnbull:
Olivier Galibert writes:
> The only part I'm really doubtful about is the ucs->byte split in the
> -xft+mule translate_to_ics2 case. I don't even know how to have the
> code pass through there.
I think Aidan is the expert on that; I think that stuff is from his
JIT charsets work. I took a hack at explaining it in my second post
(with the header comments, especially for -xft+mule), but I'd want
Aidan's review on that.
Evaluate this in *scratch* with Mule and server-side core fonts and the code
should pass through there on the next redisplay:
(mapc
(lambda (code) (insert (format "%c " (decode-char 'ucs code))))
(loop
for i from 0 to #x10000
with just-in-time-list = nil
until (eq 20 (length just-in-time-list))
finally return just-in-time-list
do
(when (and (decode-char 'ucs i)
(eq 'jit-ucs-charset-0 (char-charset (decode-char 'ucs i))))
(push i just-in-time-list))))
If it doesn’t, check what
(face-font-instance 'default nil 'jit-ucs-charset-0)
returns; it should be an XLFD ending in -iso10646-1, and when you examine the
font using xfd(1), it should have a decent repertoire of characters (a
thousand, at least.)
--
On the quay of the little Black Sea port, where the rescued pair came once
more into contact with civilization, Dobrinton was bitten by a dog which was
assumed to be mad, though it may only have been indiscriminating. (Saki)
_______________________________________________
XEmacs-Patches mailing list
XEmacs-Patches(a)xemacs.org
http://calypso.tux.org/cgi-bin/mailman/listinfo/xemacs-patches
6228
days inactive
6231
days old
14 comments
5 participants
participants (5)
-
Aidan Kehoe -
Michael Sperber -
Olivier Galibert -
Stephen J. Turnbull -
Stephen J. Turnbull