Re: [PATCH 3/9] Implement cygw32

[Eli Zaretskii <eliz@gnu.org>]

> Date: Tue, 07 Aug 2012 01:19:27 -0700
> From: Daniel Colascione <dancol@dancol.org>
> 
> +(defun w32-using-nt ()
> +  "Return non-nil if running on a Windows NT descendant.
> +That includes all Windows systems except for 9X/Me."
> +  (getenv "SystemRoot"))

I don't like to rely on environment variables for system-type
detection.  It's all to easy to remove or add environment variables.
If there's no better way that is already there, I'd prefer a new
primitive that is based on os_subtype variable in C, or something
similar.

(Yes, I know the current code uses getenv as well, but we might as
well fix it while at that.)

> --- a/lisp/w32-fns.el
> +++ b/lisp/w32-fns.el
> @@ -26,34 +26,20 @@
>  
>  ;;; Code:
>  (require 'w32-vars)
> +(require 'w32-common-fns)

Why do we need this require?  loadup.el loads w32-common-fns before
w32-fns, so it's already loaded here.  If you need this for
compilation, won't eval-when-compile be better?

> +wchar_t*
> +to_unicode (Lisp_Object str, Lisp_Object* buf)

A minor style issue: please leave a blank between '*' and the
preceding token.

> +  *buf = code_convert_string_norecord (str, Qutf_16_le, 1);
> +  /* We need to make a another copy (in addition to the one made by
> +     code_convert_string_norecord) to ensure that the final string is
> +     _doubly_ zero terminated --- that is, that the string is
> +     terminated by two zero bytes and one utf-16le null character.
> +     Because strings are already terminated with a single zero byte,
> +     we just add one additional zero. */
> +  str = make_uninit_string (SBYTES (*buf) + 1);
> +  memcpy (SDATA (str), SDATA (*buf), SBYTES (*buf));
> +  SDATA (str) [SBYTES (*buf)] = '\0';
> +  *buf = str;

Can't you append a zero byte to the original str, before encoding it
with code_convert_string_norecord?  Copying it after encoding looks
inelegant.

> +DEFUN ("cygwin-convert-path-to-windows",
> +       Fcygwin_convert_path_to_windows, Scygwin_convert_path_to_windows,
> +       1, 2, 0,
> +       doc: /* Convert PATH to a Windows path.  If ABSOLUTE-P if
> +               non-nil, return an absolute path.*/)
> +  (Lisp_Object path, Lisp_Object absolute_p)
> +{
> +  return from_unicode (
> +    conv_filename_to_w32_unicode (path, absolute_p == Qnil ? 0 : 1));
> +}

This function is only used from C.  Do we really need its Lisp
binding?

> +DEFUN ("cygwin-convert-path-from-windows",
> +       Fcygwin_convert_path_from_windows, Scygwin_convert_path_from_windows,
> +       1, 2, 0,
> +       doc: /* Convert a Windows path to a Cygwin path.  If ABSOLUTE-P
> +               if non-nil, return an absolute path.*/)
> +  (Lisp_Object path, Lisp_Object absolute_p)
> +{
> +  return conv_filename_from_w32_unicode (to_unicode (path, &path),
> +                                         absolute_p == Qnil ? 0 : 1);
> +}

I wonder why this couldn't be implemented in Lisp.  Isn't it just
decoding from UTF-16 followed by mirroring the backslashes?  If
there's more to it than that, please reflect that in the doc string.

> +#ifndef CYGW32_H
> +#define CYGW32_H
> +#include <config.h>
> +#include <windef.h>
> +#include <sys/cygwin.h>
> +#include <wchar.h>
> +
> +#include <signal.h>
> +#include <stdio.h>
> +#include <limits.h>
> +#include <errno.h>
> +#include <math.h>
> +#include <setjmp.h>
> +
> +#include "lisp.h"
> +#include "coding.h"

I think it's a bad mojo for a header to include other Emacs headers.
Is it really needed?

> +/* Access the wide-character string stored in a Lisp string object.  */
> +#define WCSDATA(x) ((wchar_t*) SDATA (x))

This is IMO yucky, and I'd like to avoid this if possible.  Can you
explain why it is needed?

> +#if defined(CYGWIN) && defined(HAVE_NTGUI)
> +#define NTGUI_UNICODE /* Cygwin runs only on UNICODE-supporting systems */
> +#define _WIN32_WINNT 0x500 /* Win2k */
> +#endif /* CYGWIN && HAVE_NTGUI */

I'd like to make tests of NTGUI_UNICODE at run time rather than at
compile time.  That's because we are gradually moving Emacs to using
Unicode APIs when available, so making these tests at run time will
allow to share more code between the Cygwin and native Windows builds.

There's already a variable w32_console_unicode_input in w32inevt.c
that is used for a similar purpose, and I believe there will be soon
another for a GUI input.  Can you define a similar variable for menus
or whatever, and either initialize it on some file that is compiled
both in the native and the Cygwin builds, or just unconditionally set
it to non-zero in some Cygwin specific function?  Then test it
whenever you need a Unicode API.

> --- a/src/emacs.c
> +++ b/src/emacs.c
> @@ -37,9 +37,20 @@ along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.  */
>  
>  #ifdef WINDOWSNT
>  #include <fcntl.h>
> -#include <windows.h> /* just for w32.h */

It looks like you are deleting the inclusion of windows.h.  Why?

> -#if defined (WINDOWSNT)
> +#if defined (HAVE_NTGUI)
>    LANGUAGE_CHANGE_EVENT,	/* A LANGUAGE_CHANGE_EVENT is
> -				   generated on WINDOWSNT or Mac OS
> +				   generated on HAVE_NTGUI or Mac OS

"On HAVE_NTGUI" does not sound right...

Thanks.