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] system-type cygwin with window-system w32 Date: Mon, 18 Jul 2011 02:13:40 -0400 Message-ID: References: <4E2377E2.1020804@gmail.com> Reply-To: Eli Zaretskii NNTP-Posting-Host: lo.gmane.org X-Trace: dough.gmane.org 1310969640 15584 80.91.229.12 (18 Jul 2011 06:14:00 GMT) X-Complaints-To: usenet@dough.gmane.org NNTP-Posting-Date: Mon, 18 Jul 2011 06:14:00 +0000 (UTC) Cc: emacs-devel@gnu.org To: Daniel Colascione Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Mon Jul 18 08:13:56 2011 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([140.186.70.17]) by lo.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1Qih5Y-0008Cz-7Q for ged-emacs-devel@m.gmane.org; Mon, 18 Jul 2011 08:13:56 +0200 Original-Received: from localhost ([::1]:53265 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qih5W-0004PH-U2 for ged-emacs-devel@m.gmane.org; Mon, 18 Jul 2011 02:13:55 -0400 Original-Received: from eggs.gnu.org ([140.186.70.92]:46030) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qih5J-0004P2-LJ for emacs-devel@gnu.org; Mon, 18 Jul 2011 02:13:42 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Qih5I-0001NU-Ni for emacs-devel@gnu.org; Mon, 18 Jul 2011 02:13:41 -0400 Original-Received: from fencepost.gnu.org ([140.186.70.10]:46718) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qih5I-0001NQ-Kq for emacs-devel@gnu.org; Mon, 18 Jul 2011 02:13:40 -0400 Original-Received: from eliz by fencepost.gnu.org with local (Exim 4.71) (envelope-from ) id 1Qih5I-0008A6-J3; Mon, 18 Jul 2011 02:13:40 -0400 In-reply-to: <4E2377E2.1020804@gmail.com> (message from Daniel Colascione on Sun, 17 Jul 2011 17:01:38 -0700) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6 (newer, 3) X-Received-From: 140.186.70.10 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:142086 Archived-At: > Date: Sun, 17 Jul 2011 17:01:38 -0700 > From: Daniel Colascione > > This patch makes it possible to use the NT GUI with a Cygwin core Emacs. Thanks. A few comments: > === modified file 'lib/filemode.c' > --- lib/filemode.c 2011-02-20 10:51:50 +0000 > +++ lib/filemode.c 2011-07-16 11:34:30 +0000 Why did you need to change filemode.c? Does it have anything to do with Cygwin on w32? > === modified file 'lisp/term/w32-win.el' > --- lisp/term/w32-win.el 2011-05-04 14:03:16 +0000 > +++ lisp/term/w32-win.el 2011-07-17 23:04:45 +0000 These look just white-space changes. If so, please leave them out of the patch. > +(defconst w32-clipboard-format-html > + (w32-register-clipboard-format "HTML Format") > + "The system-specific numeric ID of the HTML clipboard format.") > + > +(defconst w32-clipboard-html-header > + (concat "Version:0.9\r\n" > + "StartHTML:%0006d\r\n" > + "EndHTML:%0006d\r\n" > + "StartFragment%0006d\r\n" > + "EndFragment:%0006d\r\n")) > + > +(defconst w32-clipboard-html-fragment-prefix > + (concat "\r\n" > + "\r\n" > + "\r\n" > + "" > +)) What is this (and related) stuff about? Why do you need to use HTML wrt the clipboard? > +#define t(...) \ > + ({ \ > + fprintf (stderr, "T:%s:%u: ", \ > + __FUNCTION__, __LINE__); \ > + fprintf (stderr, __VA_ARGS__); \ > + fputc ('\n', stderr); \ > + }) > + What is this stuff about? > -/* Equivalent of strerror for W32 error codes. */ > -char * > -w32_strerror (int error_no) > -{ > - static char buf[500]; I don't like the idea of moving this to w32fns.c, because it doesn't belong there. Can you come up with an alternative idea? > +#if EMACSDEBUG > +const char* > +w32_name_of_message (UINT msg) Why is this needed? > + > + /* DebPrint (("w32_msg_pump: %s time:%u\n", */ > + /* w32_name_of_message (msg.message), msg.time)); */ > + Can this be removed? These DebPrint messages are a PITA when debugging, so if it isn't absolutely necessary, let's not add new ones. This is based on reviewing only a part of the patch, I will have more later. The patch is very large and complicated, and the lack of a ChangeLog that describes the changes, particularly those which move code between different files, does not help...