Re: [PATCH] GnuTLS support on Woe32

[Eli Zaretskii]

> From: Juanma Barranquero <address@hidden>
> Date: Tue, 3 May 2011 04:27:55 +0200
> Cc: address@hidden
> 
> Please take a look at the attached patch, which is a rough cut

It's mostly a mechanical change, and looks fine to me upon first
reading.

However, this:

> --- src/w32.h 2011-04-25 01:30:51 +0000
> +++ src/w32.h 2011-05-02 01:47:27 +0000
> @@ -1,6 +1,8 @@
>  #ifndef EMACS_W32_H
>  #define EMACS_W32_H
>  
> +#include "lisp.h"

is bad: lisp.h is already included in too many places.  Why did you
need this?

Also, this:

> +/* The argument LIBRARIES is usually the variable
> +   `dynamic-library-alist', which associates a symbol, identifying
> +   an external DLL library, to a list of possible filenames.
> +   The function returns NULL if no library could be loaded for
> +   the given symbol, or if the library was previously loaded;
> +   else the handle of the DLL.  */
> +HMODULE
> +w32_delayed_load (Lisp_Object libraries, Lisp_Object library_id)

has a couple of problems in the commentary:

 . it should describe the structure of the LIBRARIES alist explicitly,
   like you'd do in a doc string, not just refer to
   `dynamic-library-alist'

 . it says nothing about the second argument (and IIUC what it means,
   the "_id" part of the variable name is misleading)

Btw, why is it a good idea to return NULL if the library is already
loaded?  Why not return its handle instead?