Re: [PATCH 3/9] Implement cygw32

[Eli Zaretskii]

> Date: Tue, 07 Aug 2012 13:11:36 -0700
> From: Daniel Colascione <address@hidden>
> CC: address@hidden
> 
> Thanks for taking a look at the code.

Thanks for writing it in the first place.

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

Thanks.  let me know if you need help.

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

OK, but then please at least use make_specified_string to enlarge the
encoded string, instead of repeating what it does in-line.

Alternatively, you could avoid the extra copy in a way similar to what
w32select.c does:

  setup_windows_coding_system (coding_system, &coding);
  coding.dst_bytes = SBYTES (current_text) * 2;
  coding.destination = xmalloc (coding.dst_bytes);
  encode_coding_object (&coding, current_text, 0, 0,
                        SCHARS (current_text), SBYTES (current_text), Qnil);

After this, you have the encoded string in coding.destination.  If you
make sure the malloc'ed buffer is large enough to allow one more byte
for the extra null (which the above snippet doesn't do, btw), you are
home free.

I'll leave it up to you to decide which method is more elegant.

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

OK.

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

Most Emacs maintainers are not users of Cygwin, and they still need to
know something about the functions we provide (e.g., to be able to
review patches ;-).  The additional information that will suffice is
just what you said above: that the conversion consults Cygwin mount
tables to convert Windows drive letters to Posix file names.

And btw, please don't use "path" here, but "filename".  GNU coding
standards frown on using "path" for anything but PATH-style directory
lists.

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

It makes dependency checking harder and more error prone.

None of the Emacs headers includes config.h or lisp.h, so please move
those at least to the .c file.

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

Like we do with any encoded string: a unibyte string.

> When was the last time somebody even tested Emacs on 9X?

AFAIK, 9 months ago.  See bug #8562.

> I'm not interested in having the Cygwin build detect UNICODE at runtime.

You don't need to: you can unconditionally set some flag variable to a
non-zero value in the Cygwin initialization code, and then test that
variable where you need to use either the ANSI or the Unicode APIs.
The native Windows port, OTOH, will set that variable as appropriate
 for the underlying platform.

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

I see that.  But what I'm asking is to make one more step, and make
that test at run time, rather than at compile time.  The advantage of
that is that the code you wrote will then be used by the native
Windows build as well, and will get much more testing and user
feedback than if it were an obscure Cygwin-only feature.  I hope you
will agree that it's a win-win situation.

Thanks.