GNU bug report logs - #45550
[PATCH] Factor out new function for readability in chartab.c

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 45550 in the body.
You can then email your comments to 45550 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefan <at> marxist.se>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] Factor out new function for readability in chartab.c
Date: Wed, 30 Dec 2020 02:56:53 -0600

[Message part 1 (text/plain, inline)]

Severity: wishlist

While poking around, I found an opportunity for refactoring in
charctab.c.  I find it makes these functions significantly easier to
read and understand.

Please see the attached patch.

[0001-Factor-out-new-function-for-readability.patch (text/x-diff, attachment)]

Message #8 received at 45550 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Kangas <stefan <at> marxist.se>
Cc: 45550 <at> debbugs.gnu.org
Subject: Re: bug#45550: [PATCH] Factor out new function for readability in
 chartab.c
Date: Wed, 30 Dec 2020 22:15:14 +0200

> From: Stefan Kangas <stefan <at> marxist.se>
> Date: Wed, 30 Dec 2020 02:56:53 -0600
> 
> While poking around, I found an opportunity for refactoring in
> charctab.c.  I find it makes these functions significantly easier to
> read and understand.
> 
> Please see the attached patch.

Thanks, I have a couple of comments:

> +static Lisp_Object
> +sub_char_table_ref_and_range (Lisp_Object, int, int *, int *,
> +			      Lisp_Object, bool);

There should be no need to declare a static function which is defined
before it is used.

> +static Lisp_Object
> +char_table_ref_simple (Lisp_Object table, int idx, int c, int *from, int *to,
> +		       Lisp_Object defalt, bool is_uniprop, bool is_subtable)
> +{
> +  Lisp_Object val = is_subtable ?
> +    XSUB_CHAR_TABLE (table)->contents[idx]:
> +    XCHAR_TABLE (table)->contents[idx];
> +  if (is_uniprop && UNIPROP_COMPRESSED_FORM_P (val))
> +    val = uniprop_table_uncompress (table, idx);
> +  if (SUB_CHAR_TABLE_P (val))
> +    val = sub_char_table_ref_and_range (val, c, from, to,
> +					defalt, is_uniprop);
> +  else if (NILP (val))
> +    val = defalt;
> +  return val;
> +}
> +

Please make this a macro, not a function.  The code you are factoring
out is called in the innermost loops of the display engine, in bidi.c,
so it must be as fast as possible, even in an unoptimized build, where
static functions aren't inlined.

Message #11 received at 45550 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefan <at> marxist.se>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 45550 <at> debbugs.gnu.org
Subject: Re: bug#45550: [PATCH] Factor out new function for readability in
 chartab.c
Date: Wed, 30 Dec 2020 15:16:05 -0600

Eli Zaretskii <eliz <at> gnu.org> writes:

> Please make this a macro, not a function.  The code you are factoring
> out is called in the innermost loops of the display engine, in bidi.c,
> so it must be as fast as possible, even in an unoptimized build, where
> static functions aren't inlined.

Would it be acceptable to use our INLINE macro here instead?  I see that
it is used quite a lot in dispextern.c.  Otherwise, I can of course
rewrite it to use a macro instead.

Message #14 received at 45550 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Kangas <stefan <at> marxist.se>
Cc: 45550 <at> debbugs.gnu.org
Subject: Re: bug#45550: [PATCH] Factor out new function for readability in
 chartab.c
Date: Thu, 31 Dec 2020 05:29:08 +0200

> From: Stefan Kangas <stefan <at> marxist.se>
> Date: Wed, 30 Dec 2020 15:16:05 -0600
> Cc: 45550 <at> debbugs.gnu.org
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> > Please make this a macro, not a function.  The code you are factoring
> > out is called in the innermost loops of the display engine, in bidi.c,
> > so it must be as fast as possible, even in an unoptimized build, where
> > static functions aren't inlined.
> 
> Would it be acceptable to use our INLINE macro here instead?

I'd prefer a simple macro.  I can never remember the exact semantics
of INLINE this week.

Thanks.

Message #17 received at 45550 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 45550 <at> debbugs.gnu.org, Stefan Kangas <stefan <at> marxist.se>
Subject: Re: bug#45550: [PATCH] Factor out new function for readability in
 chartab.c
Date: Wed, 21 Jul 2021 14:03:40 +0200

Eli Zaretskii <eliz <at> gnu.org> writes:

>> > Please make this a macro, not a function.  The code you are factoring
>> > out is called in the innermost loops of the display engine, in bidi.c,
>> > so it must be as fast as possible, even in an unoptimized build, where
>> > static functions aren't inlined.
>> 
>> Would it be acceptable to use our INLINE macro here instead?
>
> I'd prefer a simple macro.  I can never remember the exact semantics
> of INLINE this week.

I've applied Stefan's patch (with the "static inline" tweak, which we
use other places in Emacs, too) because patches bit-rot when they get
too old.  If it turns out to be a performance issue, then the inline
function can be rewritten as a macro.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.