Code inspection: W32 specific problems

Code inspection: W32 specific problems

[Kim F. Storm]

Code inspection revealed the following w32 specific problems:

- Qhigh and Qlow have no staticpro.

- Vw32_valid_codepages has no staticpro.

- Vw32_valid_locale_ids has no staticpro.

- Qw32_charset_default is never initialized, but its value
  is used in two places.

- Vx_hand_shape is declared, but never initialized or used.


Can someone pls. look at this and fix the problems, or
alternatively specify why staticpro's are not needed.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: Code inspection: W32 specific problems

[Eli Zaretskii]

> From: storm@cua.dk (Kim F. Storm)
> Date: Tue, 12 Jul 2005 14:44:06 +0200
> 
> 
> Code inspection revealed the following w32 specific problems:
> 
> - Qhigh and Qlow have no staticpro.
> 
> - Vw32_valid_codepages has no staticpro.
> 
> - Vw32_valid_locale_ids has no staticpro.
> 
> - Qw32_charset_default is never initialized, but its value
>   is used in two places.
> 
> - Vx_hand_shape is declared, but never initialized or used.
> 
> 
> Can someone pls. look at this and fix the problems, or
> alternatively specify why staticpro's are not needed.

Can please you remind the rules for what variables need staticpro?

Re: Code inspection: W32 specific problems

[Stefan Monnier]

> Can please you remind the rules for what variables need staticpro?

All static/global variables of type Lisp_Object.
The `staticpro' simply adds the variable to the list of root pointers for
the GC, so if the staticpro is missing, the GC may accidentally collect the
object, even though it's live.

If you want to live dangerously, you can try to argue in some corner cases
that a staticpro is not necessary by showing that in all possible cases,
the object stored in the variable is either non-allocated (e.g. a fixnum),
or will always be reachable via some other way (which the GC will use to
detect that the object is live).

I hope nobody here wants to live dangerously, since in this case it's a lot
more trouble than it's worth.


        Stefan

Re: Code inspection: W32 specific problems

[Kim F. Storm]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> I hope nobody here wants to live dangerously, since in this case it's a lot
> more trouble than it's worth.

I found more of such variables that I did not include in
the list because they indeed seemed to be "non-dangerous".

E.g.

being_printed[PRINT_CIRCLE]
case_temp2
composition_temp
last_marked[LAST_MARKED_SIZE]
process_sent_to
syntax_temp
_temp_category_set

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: Code inspection: W32 specific problems

[Eli Zaretskii]

> From: storm@cua.dk (Kim F. Storm)
> Date: Tue, 12 Jul 2005 14:44:06 +0200
> 
> 
> Code inspection revealed the following w32 specific problems:
> 
> - Qhigh and Qlow have no staticpro.
> 
> - Vw32_valid_codepages has no staticpro.
> 
> - Vw32_valid_locale_ids has no staticpro.
> 
> - Qw32_charset_default is never initialized, but its value
>   is used in two places.
> 
> - Vx_hand_shape is declared, but never initialized or used.
> 
> 
> Can someone pls. look at this and fix the problems, or
> alternatively specify why staticpro's are not needed.

Thanks.  I added staticpro for the first 4 variables.

As for Qw32_charset_default and Vx_hand_shape, someone with more
knowledge in the w32 code than myself should look at this.

Re: Code inspection: W32 specific problems

[David Hunter]

[-- Attachment #1: Type: text/plain, Size: 501 bytes --]

Eli Zaretskii wrote:
> As for Qw32_charset_default and Vx_hand_shape, someone with more
> knowledge in the w32 code than myself should look at this.

The declaration for Vx_hand_shape may be removed.

Qw32_charset_default was declared in w32term.c rev 1.62 in early 2000, but was not defined there either.  It is potentially used by several codepaths, notably in enum_font_cb2().  The related symbol 'w32-charset-default' also exists in documentation.  It should be defined.

Here's a patch.

-Dave



[-- Attachment #2: w32fns.c.patch --]
[-- Type: text/plain, Size: 1098 bytes --]

*** w32fns.c	4 Jul 2005 16:06:36 -0000	1.253
--- w32fns.c	16 Jul 2005 17:13:46 -0000
***************
*** 153,159 ****
     over text or in the modeline.  */
  
  Lisp_Object Vx_pointer_shape, Vx_nontext_pointer_shape, Vx_mode_pointer_shape;
! Lisp_Object Vx_hourglass_pointer_shape, Vx_window_horizontal_drag_shape, Vx_hand_shape;
  
  /* The shape when over mouse-sensitive text.  */
  
--- 153,159 ----
     over text or in the modeline.  */
  
  Lisp_Object Vx_pointer_shape, Vx_nontext_pointer_shape, Vx_mode_pointer_shape;
! Lisp_Object Vx_hourglass_pointer_shape, Vx_window_horizontal_drag_shape;
  
  /* The shape when over mouse-sensitive text.  */
  
***************
*** 8760,8765 ****
--- 8760,8767 ----
    staticpro (&Qw32_charset_ansi);
    Qw32_charset_ansi = intern ("w32-charset-ansi");
    staticpro (&Qw32_charset_symbol);
+   Qw32_charset_default = intern ("w32-charset-default");
+   staticpro (&Qw32_charset_default);
    Qw32_charset_symbol = intern ("w32-charset-symbol");
    staticpro (&Qw32_charset_shiftjis);
    Qw32_charset_shiftjis = intern ("w32-charset-shiftjis");


[-- Attachment #3: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: Code inspection: W32 specific problems

[Kim F. Storm]

David Hunter <hunterd_42@comcast.net> writes:

> Eli Zaretskii wrote:
>> As for Qw32_charset_default and Vx_hand_shape, someone with more
>> knowledge in the w32 code than myself should look at this.
>
> The declaration for Vx_hand_shape may be removed.

>
> Qw32_charset_default was declared in w32term.c rev 1.62 in early
>2000, but was not defined there either.  It is potentially used by
>several codepaths, notably in enum_font_cb2().  The related symbol
>'w32-charset-default' also exists in documentation.  It should be
>defined.

What about its initial value?
Is it always set from lisp before first usage?

>
> Here's a patch.

Eli, will you handle this?

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: Code inspection: W32 specific problems

[David Hunter]

Kim F. Storm wrote:
>>Qw32_charset_default was declared in w32term.c rev 1.62 in early
>>2000, but was not defined there either.  It is potentially used by
>>several codepaths, notably in enum_font_cb2().  The related symbol
>>'w32-charset-default' also exists in documentation.  It should be
>>defined.
> 
> What about its initial value?
> Is it always set from lisp before first usage?

Hmm, I think I might have used the wrong terms to describe the issue.  My grip on eLisp is not yet that strong (and probably never will be :) ).

Qw32_charset_default should just be a C pointer to a Lisp symbol whose name should be 'w32-charset-default'.  Note that it is used only as a symbol and not evaulated as a variable; its value is void.  It should simply be interned, so that the C variable Qw32_charset_default will be equivalent (by the EQ macro) to the Lisp symbol 'w32-charset-default'.

For reference, examine the relationship between the C variable Qw32_charset_ansi and the Lisp symbol 'w32-charset-ansi', established at w32fns.c:8794.

-Dave

Re: Code inspection: W32 specific problems

[Eli Zaretskii]

> Cc: Eli Zaretskii <eliz@gnu.org>,  emacs-devel@gnu.org
> From: storm@cua.dk (Kim F. Storm)
> Date: Mon, 18 Jul 2005 14:29:30 +0200
> 
> > Here's a patch.
> 
> Eli, will you handle this?

If no one else does, I will, when I have time.

Re: Code inspection: W32 specific problems

[Kim F. Storm]

David Hunter <hunterd_42@comcast.net> writes:

> Kim F. Storm wrote:
>>>Qw32_charset_default was declared in w32term.c rev 1.62 in early
>>>2000, but was not defined there either.  It is potentially used by
>>>several codepaths, notably in enum_font_cb2().  The related symbol
>>>'w32-charset-default' also exists in documentation.  It should be
>>>defined.
>> What about its initial value?
>> Is it always set from lisp before first usage?
>
> Hmm, I think I might have used the wrong terms to describe the
> issue.  My grip on eLisp is not yet that strong (and probably never
> will be :) ).

No, you were correct -- I didn't look at the actual code, so
I incorrectly assumed that the code looked at the value of 
w32-charset-default (as a lisp variable).  It only uses the
symbol itself.


-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: Code inspection: W32 specific problems

[Kim F. Storm]

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: Eli Zaretskii <eliz@gnu.org>,  emacs-devel@gnu.org
>> From: storm@cua.dk (Kim F. Storm)
>> Date: Mon, 18 Jul 2005 14:29:30 +0200
>> 
>> > Here's a patch.
>> 
>> Eli, will you handle this?
>
> If no one else does, I will, when I have time.

I installed the changes.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: Code inspection: W32 specific problems

[Eli Zaretskii]

> Cc: hunterd_42@comcast.net,  emacs-devel@gnu.org
> From: storm@cua.dk (Kim F. Storm)
> Date: Tue, 19 Jul 2005 10:55:11 +0200
> 
> I installed the changes.

Thank you.