Re: [PATCH 3/9] Implement cygw32

[Daniel Colascione]

Thanks for taking a look at the code.

On 8/7/2012 11:02 AM, Eli Zaretskii wrote:
>> Date: Tue, 07 Aug 2012 01:19:27 -0700
>> From: Daniel Colascione <address@hidden>
>>
>> +(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.

I'll see what I can do.

> 
> (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?

We require w32-common-fns here because we require functions from w32-common-fns.
I don't like writing a module that relies on _other_ modules being gracious
enough to load its own dependencies.

>> +wchar_t*
>> +to_unicode (Lisp_Object str, Lisp_Object* buf)
> 
> A minor style issue: please leave a blank between '*' and the
> preceding token.

Sure.

> 
>> +  *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.

I don't think code_convert_string_norecord is _always_ guaranteed to make a
copy, and I don't want to modify the input string. This function isn't on a hot
path, and the extra copy adds safety. I can explain in the command why we don't
do what you suggest above.

> 
>> +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?

The function is generally useful for all sorts of things. If this functionality
isn't expose through lisp, lisp code would have to exec cygpath(1), and that's
going to be much slower than just using the Cygwin function in-process.

> 
>> +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.

Cygwin has its own mount table, and converting between Cygwin and Windows paths
is non-trivial. Anyone who uses Cygwin will know about the differences between
Cygwin and Windows paths, and I'm not sure what additional information in the
docstring would be warranted.

> 
>> +#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?

I think it's bad mojo for a header _not_ to include what it needs. We have
include guards for a reason.

>> +/* 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?

We need wide-character strings. What other data type do you propose using for
the purpose?

>> +#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.

Look: I'd be 100% in favor of making Emacs UNICODE-only. In fact, the first
version of this patch did exactly that. You and RMS rejected the idea because we
want to be platform necrophiliacs and support Windows 9X. I have to repeat that
nobody actually uses those systems and that we're wasting our time with that
support. When was the last time somebody even tested Emacs on 9X?

In the current version of the patch, NTGUI_UNICODE means "assume at compile time
that we will run on a system with UNICODE support". This constant allows us to
make certain simplifying assumptions, especially in the Cygwin path conversion
code. !NTGUI_UNICODE means "use Unicode APIs when available".

I'm not interested in having the Cygwin build detect UNICODE at runtime. There's
no such thing as a non-UNICODE Cygwin 1.7 environment, and Emacs only supports
Cygwin 1.7 or better.

> 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.

We already do that for menus --- in !NTGUI_UNICODE builds, the variable
unicode_append_menu is bound to AppendMenuW if available. In NTGUI_UNICODE
builds, unicode_append_menu _is_ AppendMenuW.

> 
>> --- 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?

emacs.c doesn't need it.

> 
>> -#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...

The phrasing is correct, but grammatically contorted. I'll fix it.

signature.asc
Description: OpenPGP digital signature