From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Daniel Colascione Newsgroups: gmane.emacs.devel Subject: Re: [PATCH 3/9] Implement cygw32 Date: Tue, 07 Aug 2012 13:11:36 -0700 Message-ID: <50217678.7010106@dancol.org> References: <83obmmfunb.fsf@gnu.org> NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="------------enigED2745BC5349287541740217" X-Trace: dough.gmane.org 1344370333 8235 80.91.229.3 (7 Aug 2012 20:12:13 GMT) X-Complaints-To: usenet@dough.gmane.org NNTP-Posting-Date: Tue, 7 Aug 2012 20:12:13 +0000 (UTC) Cc: emacs-devel@gnu.org To: Eli Zaretskii Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Tue Aug 07 22:12:12 2012 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1Syq8Q-0005p6-IG for ged-emacs-devel@m.gmane.org; Tue, 07 Aug 2012 22:12:10 +0200 Original-Received: from localhost ([::1]:39094 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Syq8P-0002b7-J4 for ged-emacs-devel@m.gmane.org; Tue, 07 Aug 2012 16:12:09 -0400 Original-Received: from eggs.gnu.org ([208.118.235.92]:45631) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Syq8M-0002ZF-CC for emacs-devel@gnu.org; Tue, 07 Aug 2012 16:12:07 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Syq8J-0006YZ-EJ for emacs-devel@gnu.org; Tue, 07 Aug 2012 16:12:06 -0400 Original-Received: from dancol.org ([96.126.100.184]:38675) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Syq8G-0006Xv-P6; Tue, 07 Aug 2012 16:12:00 -0400 Original-Received: from c-76-22-66-162.hsd1.wa.comcast.net ([76.22.66.162] helo=[0.0.0.0]) by dancol.org with esmtpsa (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.72) (envelope-from ) id 1Syq8F-0007pe-JD; Tue, 07 Aug 2012 13:11:59 -0700 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0) Gecko/20120428 Thunderbird/12.0.1 In-Reply-To: <83obmmfunb.fsf@gnu.org> X-Enigmail-Version: 1.4.2 X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 96.126.100.184 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.devel:152287 Archived-At: This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enigED2745BC5349287541740217 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable 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 >> >> +(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")) >=20 > 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. >=20 > (Yes, I know the current code uses getenv as well, but we might as > well fix it while at that.) >=20 >> --- a/lisp/w32-fns.el >> +++ b/lisp/w32-fns.el >> @@ -26,34 +26,20 @@ >> =20 >> ;;; Code: >> (require 'w32-vars) >> +(require 'w32-common-fns) >=20 > 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-comm= on-fns. I don't like writing a module that relies on _other_ modules being gracio= us enough to load its own dependencies. >> +wchar_t* >> +to_unicode (Lisp_Object str, Lisp_Object* buf) >=20 > A minor style issue: please leave a blank between '*' and the > preceding token. Sure. >=20 >> + *buf =3D 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 =3D make_uninit_string (SBYTES (*buf) + 1); >> + memcpy (SDATA (str), SDATA (*buf), SBYTES (*buf)); >> + SDATA (str) [SBYTES (*buf)] =3D '\0'; >> + *buf =3D str; >=20 > 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. >=20 >> +DEFUN ("cygwin-convert-path-to-windows", >> + Fcygwin_convert_path_to_windows, Scygwin_convert_path_to_windo= ws, >> + 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 =3D=3D Qnil ? 0 : = 1)); >> +} >=20 > 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 functio= nality isn't expose through lisp, lisp code would have to exec cygpath(1), and t= hat's going to be much slower than just using the Cygwin function in-process. >=20 >> +DEFUN ("cygwin-convert-path-from-windows", >> + Fcygwin_convert_path_from_windows, Scygwin_convert_path_from_w= indows, >> + 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 =3D=3D Qnil ? 0 := 1); >> +} >=20 > 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 be= tween Cygwin and Windows paths, and I'm not sure what additional information in= the docstring would be warranted. >=20 >> +#ifndef CYGW32_H >> +#define CYGW32_H >> +#include >> +#include >> +#include >> +#include >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#include "lisp.h" >> +#include "coding.h" >=20 > 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 hav= e include guards for a reason. >> +/* Access the wide-character string stored in a Lisp string object. = */ >> +#define WCSDATA(x) ((wchar_t*) SDATA (x)) >=20 > 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 syste= ms */ >> +#define _WIN32_WINNT 0x500 /* Win2k */ >> +#endif /* CYGWIN && HAVE_NTGUI */ >=20 > 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 fir= st version of this patch did exactly that. You and RMS rejected the idea bec= ause we want to be platform necrophiliacs and support Windows 9X. I have to repea= t that nobody actually uses those systems and that we're wasting our time with t= hat support. When was the last time somebody even tested Emacs on 9X? In the current version of the patch, NTGUI_UNICODE means "assume at compi= le 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 conve= rsion 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 sup= ports 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_UNICOD= E builds, unicode_append_menu _is_ AppendMenuW. >=20 >> --- a/src/emacs.c >> +++ b/src/emacs.c >> @@ -37,9 +37,20 @@ along with GNU Emacs. If not, see . */ >> =20 >> #ifdef WINDOWSNT >> #include >> -#include /* just for w32.h */ >=20 > It looks like you are deleting the inclusion of windows.h. Why? emacs.c doesn't need it. >=20 >> -#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 >=20 > "On HAVE_NTGUI" does not sound right... The phrasing is correct, but grammatically contorted. I'll fix it. --------------enigED2745BC5349287541740217 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.12 (Cygwin) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iEYEARECAAYFAlAhdnoACgkQ17c2LVA10VtndACgxbrGyIhmqaZosAq8dHtw3Cny VJ8AnRcRAzrItWLyeaNgMtBAG3RDEyHN =AEK+ -----END PGP SIGNATURE----- --------------enigED2745BC5349287541740217--