Re: [Q] Static checker fun
"Stephen J. Turnbull" <stephen(a)xemacs.org> wrote:
QUERY
Sorry for the late call! But....
Fair warning: I'm still planning to look at text.c later.
Okay.
>>>>> "Jerry" == Jerry James
<james(a)xemacs.org> writes:
Jerry> window.c: CURCHARSIZE expands to a call to either
Jerry> window_char_width or window_char_height, depending on the
Jerry> value of widthflag. Since the code visible in this patch
Jerry> is inside of a 'if (widthflag) { ... }' block, there is
Jerry> really no point in making that comparison.
I tend to disagree. The series of macros including CURCHARSIZE was
clearly designed to be optimized away in cases like this. You can
disagree with that design, but if so you should also expand and
simplify the immediately preceding call the CURSIZE. The CURSIZE
macro itself is rather bizarre; the storage pointed to is never used
as an lvalue, so why it is defined to resolve to an int* rather than a
int is really unclear to me.
(I wonder if catching CURCHARSIZE but not CURSIZE should be considered
a bug in the static checker?)
First, note that window_char_height is called directly (i.e., without
the macro) in both redisplay.c and window.c, so the abstraction is
already broken. Second, it was *after* I expanded CURCHARSIZE that the
static checker then complained about CHARSIZE. Weird, huh? I guess it
was working its way backward up the dataflow path and complained about
the first item it saw that seemed wrong to it.
If you would like me to put those macros back, that's fine. As you
note, a good optimizing compiler should do the job, anyway.
And that CURSIZE macro is bizarre. Would you object to me "fixing" it
to be an rvalue? As it stands, it's violating the C99 aliasing rules,
apparently for no reason.
There are a number of places in XEmacs where macros depend on tests
whose result can be determined in advance; they are often defensive
programming against future extension (ie, tests that a buffer text
unit be 0 <= c < 256 when the unit is a char) or intended to be
optimized away (as here). If we're going to break the design, maybe
we should be consistent about it and eliminate such macros everywhere?
I think it would probably be best if you separate out changes that
require "looking inside" macro calls like this into a separate patch.
Okay.
(Indeed I do get sick of trying to figure out this kind of pseudo-
generality when it's only used in one place!)
:-) Thanks for the review.
--
Jerry James, Assistant Professor james(a)xemacs.org
Computer Science Department http://www.cs.usu.edu/~jerry/
Utah State University