bug#12463: 24.2; pos-visible-in-window-p gets slower over time

[Eli Zaretskii <eliz@gnu.org>]

> From: Chong Yidong <cyd@gnu.org>
> Date: Fri, 21 Sep 2012 13:42:53 +0800
> Cc: Jörg Walter <jwalt@garni.ch>, 12463@debbugs.gnu.org
> 
> Please take a look at the following patch, which moves all the work done
> by Finit_image_library into lookup_image_type.  This requires adding a
> LIBRARIES argument to lookup_image_type, with the same meaning as
> Finit_image_library and defaulting to Vdynamic_library_alist.  Then
> Finit_image_library would be a rather simple wrapper around
> lookup_image_type.  Other callers to lookup_image_type, such as
> redisplay looking up an image, would be unaffected.

Thanks.  Some comments below.

>   /* Define a new image type from TYPE.  This adds a copy of TYPE to
>      image_types and caches the loading status of TYPE.  */

The LIBRARIES argument should be documented in the commentary.

> ! static struct image_type *
> ! define_image_type (struct image_type *type, Lisp_Object libraries)
>   {

Since LIBRARIES could now be Qnil, but this function cannot tolerate
that, there should be an assertion to that effect, either here or in
every type->init function.

> ! #ifdef HAVE_NTGUI
> !       /* If we failed to load the library before, don't try again.  */
> !       Lisp_Object tested = Fassq (target_type, Vlibrary_cache);
> !       if (CONSP (tested) && NILP (XCDR (tested)))
> ! 	type_valid = 0;
> !       else
> ! #endif
> ! 	{
> ! 	  /* If the load failed, avoid trying again.  */
> ! 	  type_valid = (*type->init)(libraries);
> ! 	  CACHE_IMAGE_TYPE (target_type, type_valid);
> ! 	}
> !     }

What will happen if 'tested' is not a cons cell?

>   of `dynamic-library-alist', which see).  */)
>     (Lisp_Object type, Lisp_Object libraries)
>   {
> !   struct image_type *p = lookup_image_type (type, libraries);
> !   return p ? *p->type : Qnil;
> ! }

This changes the return value of init-image-library; is there a good
reason for not returning Qt here instead of the type symbol?

> ! /* Look up image type SYMBOL, and return a pointer to its image_type
> !    structure.  Value is null if SYMBOL is not a known image type.  */

Again, LIBRARIES is not documented.  Also, I believe we use NULL in
caps elsewhere.  And finally, the argument is called TYPE, not SYMBOL.

> ! static struct image_type *
> ! lookup_image_type (Lisp_Object type, Lisp_Object libraries)
> ! {
> !   if (NILP (libraries))
> !     libraries = Vdynamic_library_alist;

I can't say I like this "default".  Why not always call
lookup_image_type with Vdynamic_library_alist?  For that matter, why
not make lookup_image_type always use Vdynamic_library_alist without
passing it through the call parameters?