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: Some comments about changes in revno 110444 Date: Mon, 08 Oct 2012 05:53:52 -0700 Message-ID: <5072CCE0.9000108@dancol.org> References: <83sj9p448h.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="------------enigE9EB312AD448033E3A72B5D8" X-Trace: ger.gmane.org 1349700857 31959 80.91.229.3 (8 Oct 2012 12:54:17 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Mon, 8 Oct 2012 12:54:17 +0000 (UTC) Cc: emacs-devel@gnu.org To: Eli Zaretskii Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Mon Oct 08 14:54:23 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 1TLCql-00068l-AF for ged-emacs-devel@m.gmane.org; Mon, 08 Oct 2012 14:54:23 +0200 Original-Received: from localhost ([::1]:43455 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TLCqf-0008Qk-1l for ged-emacs-devel@m.gmane.org; Mon, 08 Oct 2012 08:54:17 -0400 Original-Received: from eggs.gnu.org ([208.118.235.92]:58704) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TLCqb-0008Pz-PA for emacs-devel@gnu.org; Mon, 08 Oct 2012 08:54:15 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TLCqX-0007sa-BL for emacs-devel@gnu.org; Mon, 08 Oct 2012 08:54:13 -0400 Original-Received: from dancol.org ([96.126.100.184]:55979) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TLCqN-0007qS-2V; Mon, 08 Oct 2012 08:53:59 -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 1TLCqL-0005E2-85; Mon, 08 Oct 2012 05:53:57 -0700 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0) Gecko/20120428 Thunderbird/12.0.1 In-Reply-To: <83sj9p448h.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:154229 Archived-At: This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enigE9EB312AD448033E3A72B5D8 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable On 10/8/2012 4:01 AM, Eli Zaretskii wrote: > This hunk removes w32inevt from SOME_MACHINE_OBJECTS. Why was that > needed? Merge problem. Fixed. > To avoid confusion, I suggest to change the name of the variable here: >=20 > + /* No, not utf-16-le: that one has a BOM. */ > + DEFSYM (Qutf_16_le, "utf-16le"); >=20 > to Qutf_16le. Good catch. > Isn't the below supposed to be done automagically by make-docfile, > which puts these in globals.h? >=20 > +EXFUN (Fcygwin_convert_path_to_windows, 2); > +EXFUN (Fcygwin_convert_path_from_windows, 2); The earliest version of this code targeted Emacs 23, which doesn't have a= globals.h. Fixed. > I don't think this (from w32select.h) is a good idea: >=20 > +#define HAVE_W32SELECT 1 >=20 > We only have definitions for HAVE_* macros in config.h and > conf_post.h. Having them in other headers is confusing, IMO. Can we > move this to conf_post.h instead? We only used HAVE_W32SELECT in two places, so I just replaced HAVE_W32SEL= ECT with the conditional we use to include the header --- WINDOWSNT || HAVE_N= TGUI. > You moved the definitions of these from w32heap.c to w32fns.c: >=20 > +/* The major and minor versions of NT. */ > +int w32_major_version; > +int w32_minor_version; > +int w32_build_number; >=20 > but they are still referenced by w32.c via get_* macros defined on > w32heap.h. Can we clean this up, please? The bigger issue is that w32heap.c is WINDOWSNT-only, but w32heap.h decla= res useful facilities that apply more generally (and that have nothing to do = with the heap). I created a new w32common.h for these common declarations and = left the WINDOWSNT-specific heap stuff in w32heap.h. > Regarding this: >=20 > +#ifdef NTGUI_UNICODE > + wchar_t filename_buf[MAX_PATH + 1]; >=20 > I believe Unicode names on Windows are not limited to MAX_PATH > characters. You can use special syntax to exceed that limit --- http://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=3Dvs.8= 5).aspx --- but nobody does in practice. Bear in mind that this code only deals = with the file open dialog box, which (the last time I checked) didn't support = long file names in the first place. Still, I've increased the limit to 32k + 1, which is a hard NT kernel lim= it, in the NTGUI_UNICODE case. > It looks like there are some unnecessary hops here: >=20 > + CHECK_STRING (prompt); > + CHECK_STRING (dir); > + > + dir =3D Fexpand_file_name (dir, Qnil); > + > + if (STRINGP (filename)) > + filename =3D Ffile_name_nondirectory (filename); > + else > + filename =3D empty_unibyte_string; > + > +#ifdef CYGWIN > + dir =3D Fcygwin_convert_path_to_windows (dir, Qt); > + if (SCHARS (filename) > 0) > + filename =3D Fcygwin_convert_path_to_windows (filename, Qnil);= > +#endif >=20 > + CHECK_STRING (dir); > + CHECK_STRING (filename); > + > + /* The code in file_dialog_callback that attempts to set the tex= t > + of the file name edit window when handling the CDN_INITDONE > + WM_NOTIFY message does not work. Setting filename to "Curren= t > + Directory" in the only_dir_p case here does work however. */= > + if (SCHARS (filename) =3D=3D 0 && ! NILP (only_dir_p)) > + filename =3D build_string ("Current Directory"); > + > + /* Convert the values we've computed so far to system form. */ > +#ifdef NTGUI_UNICODE > + to_unicode (prompt, &prompt); > + to_unicode (dir, &dir); > + to_unicode (filename, &filename); >=20 > AFAIU, inside Fcygwin_convert_path_to_windows, you convert the names > of the file and directory to UTF-16, then decode it back into a > multibyte string, and finally encode it again to UTF-16 when you call > to_unicode. Perhaps the back and forth conversion to and from UTF-16 > can be avoided? This code isn't on a hot path. I'm reluctant to optimize it: I'm not sur= e the additional complexity would be worth it. > In any case, a call to CHECK_STRING after we already checked the > arguments seems both redundant and user-unfriendly: these tests are > designed to reject errors in arguments passed to primitives, so > signaling an error because something went wrong in processing valid > arguments doesn't sound like a good idea. >=20 > This error will not report the offending file name: >=20 > + if (SBYTES (filename) + 1 > sizeof (filename_buf)) > + error ("filename too long"); >=20 > I think report_file_error is better here. Fixed. > Why do you need the fprintf part here: > #ifdef EMACSDEBUG > void > _DebPrint (const char *fmt, ...) > { > char buf[1024]; > va_list args; >=20 > va_start (args, fmt); > vsprintf (buf, fmt, args); > va_end (args); > #if CYGWIN > fprintf (stderr, "%s", buf); > #endif > OutputDebugString (buf); > } Because tracing on stderr is useful. Strings sent to OutputDebugString a= re invisible unless Emacs is being run under a debugger. > What happened to number 21 in the change below? Bad merge. Fixed. > #define WM_EMACS_HIDE_CARET (WM_EMACS_START + 18) > #define WM_EMACS_SETCURSOR (WM_EMACS_START + 19) > #define WM_EMACS_PAINT (WM_EMACS_START + 20) > -#define WM_EMACS_BRINGTOTOP (WM_EMACS_START + 21) > -#define WM_EMACS_END (WM_EMACS_START + 22) > +#define WM_EMACS_BRINGTOTOP (WM_EMACS_START + 22) > +#define WM_EMACS_INPUT_READY (WM_EMACS_START + 23) > +#define WM_EMACS_END (WM_EMACS_START + 24) >=20 > Why did you need the call to notify_msg_ready below? >=20 > @@ -285,7 +309,7 @@ prepend_msg (W32Msg *lpmsg) > nQueue++; > lpNew->lpNext =3D lpHead; > lpHead =3D lpNew; > - > + notify_msg_ready (); The call to notify_msg_ready is needed because after a call to prepend_ms= g, a message is now ready. It should have been there all along. --------------enigE9EB312AD448033E3A72B5D8 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/ iEYEARECAAYFAlByzOMACgkQ17c2LVA10VuggQCguB5Sch/jBI2qThCRyJsh/4x2 sNYAniFZq1GFojo1FJbA+aI83sqZSAlQ =zNS9 -----END PGP SIGNATURE----- --------------enigE9EB312AD448033E3A72B5D8--