From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.devel Subject: Re: [PATCH 3/9] Implement cygw32 Date: Wed, 08 Aug 2012 20:15:59 +0300 Message-ID: <831ujhfgog.fsf@gnu.org> References: <83obmmfunb.fsf@gnu.org> <50217678.7010106@dancol.org> Reply-To: Eli Zaretskii NNTP-Posting-Host: plane.gmane.org X-Trace: dough.gmane.org 1344446193 26232 80.91.229.3 (8 Aug 2012 17:16:33 GMT) X-Complaints-To: usenet@dough.gmane.org NNTP-Posting-Date: Wed, 8 Aug 2012 17:16:33 +0000 (UTC) Cc: emacs-devel@gnu.org To: Daniel Colascione Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Wed Aug 08 19:16:34 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 1Sz9s0-00015u-Lc for ged-emacs-devel@m.gmane.org; Wed, 08 Aug 2012 19:16:32 +0200 Original-Received: from localhost ([::1]:55665 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Sz9rz-0003m6-Qz for ged-emacs-devel@m.gmane.org; Wed, 08 Aug 2012 13:16:31 -0400 Original-Received: from eggs.gnu.org ([208.118.235.92]:41765) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Sz9rx-0003lo-2Z for emacs-devel@gnu.org; Wed, 08 Aug 2012 13:16:30 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Sz9rv-0001KO-HA for emacs-devel@gnu.org; Wed, 08 Aug 2012 13:16:29 -0400 Original-Received: from mtaout20.012.net.il ([80.179.55.166]:33865) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Sz9rv-0001Jx-3e for emacs-devel@gnu.org; Wed, 08 Aug 2012 13:16:27 -0400 Original-Received: from conversion-daemon.a-mtaout20.012.net.il by a-mtaout20.012.net.il (HyperSendmail v2007.08) id <0M8G0010059CAE00@a-mtaout20.012.net.il> for emacs-devel@gnu.org; Wed, 08 Aug 2012 20:15:57 +0300 (IDT) Original-Received: from HOME-C4E4A596F7 ([87.69.4.28]) by a-mtaout20.012.net.il (HyperSendmail v2007.08) with ESMTPA id <0M8G001Z25AL0E40@a-mtaout20.012.net.il>; Wed, 08 Aug 2012 20:15:57 +0300 (IDT) In-reply-to: <50217678.7010106@dancol.org> X-012-Sender: halo1@inter.net.il X-detected-operating-system: by eggs.gnu.org: Solaris 10 (beta) X-Received-From: 80.179.55.166 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:152344 Archived-At: > Date: Tue, 07 Aug 2012 13:11:36 -0700 > From: Daniel Colascione > CC: emacs-devel@gnu.org > > 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 > >> +#include > >> +#include > >> +#include > >> + > >> +#include > >> +#include > >> +#include > >> +#include > >> +#include > >> +#include > >> + > >> +#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.