From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Adrian Robert Newsgroups: gmane.emacs.devel Subject: Re: a review of the merge (Re: Emacs.app merged) Date: Thu, 17 Jul 2008 13:22:56 -0400 Message-ID: References: <1C66F1FC-BF82-4365-944D-ADCC4D1F435C@gmail.com> <200807160925.m6G9PuVj012462@sallyv1.ics.uci.edu> <200807170324.m6H3OwUW018599@sallyv1.ics.uci.edu> NNTP-Posting-Host: lo.gmane.org Mime-Version: 1.0 (Apple Message framework v926) Content-Type: text/plain; charset=US-ASCII; format=flowed; delsp=yes Content-Transfer-Encoding: 7bit X-Trace: ger.gmane.org 1216315413 10574 80.91.229.12 (17 Jul 2008 17:23:33 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Thu, 17 Jul 2008 17:23:33 +0000 (UTC) Cc: emacs- devel To: Dan Nicolaescu Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Thu Jul 17 19:24:21 2008 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([199.232.76.165]) by lo.gmane.org with esmtp (Exim 4.50) id 1KJXDJ-0000vf-0K for ged-emacs-devel@m.gmane.org; Thu, 17 Jul 2008 19:24:21 +0200 Original-Received: from localhost ([127.0.0.1]:45802 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1KJXCQ-00044E-4S for ged-emacs-devel@m.gmane.org; Thu, 17 Jul 2008 13:23:26 -0400 Original-Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1KJXCL-00043s-Dm for emacs-devel@gnu.org; Thu, 17 Jul 2008 13:23:21 -0400 Original-Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1KJXCJ-000431-Fa for emacs-devel@gnu.org; Thu, 17 Jul 2008 13:23:21 -0400 Original-Received: from [199.232.76.173] (port=46693 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1KJXCJ-00042y-Bv for emacs-devel@gnu.org; Thu, 17 Jul 2008 13:23:19 -0400 Original-Received: from an-out-0708.google.com ([209.85.132.240]:2808) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1KJXCI-0008SR-T7 for emacs-devel@gnu.org; Thu, 17 Jul 2008 13:23:19 -0400 Original-Received: by an-out-0708.google.com with SMTP id c38so208061ana.84 for ; Thu, 17 Jul 2008 10:23:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:received:received:cc:message-id:from:to :in-reply-to:content-type:content-transfer-encoding:mime-version :subject:date:references:x-mailer; bh=9WTcP8lF30vMeSSvwVYU5/Ds8zQGQi1E3xpgM40GpF8=; b=MM7aCWdQ3MJafUHa/gr7Q6D09JBe9Hyaeu06HuMmKCAWIiFXffBLwi/9PahaWmpgFD cvsvvciGkIQr97c5kChkntnv9+zK8svbx3QZjYC9JFuO0LDo89NZ7C9N4vN5M0mELjXY 0CpYAj/qfByN0sqQU5mhv7uDYbBiud5CYqWq8= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=cc:message-id:from:to:in-reply-to:content-type :content-transfer-encoding:mime-version:subject:date:references :x-mailer; b=qHkeDxu/ENOzwRIn53UI8oXfRz3BO6vflrAeHTBUWac85XzADjyunvRHRHAMqJOCVc gbTBy/BPjqeL+iARylbTSNClwieGHHBW0FpSN0D7OkpLugo0AH2ZwPWIZlylMhe7YTyw FsE0KVFs9NLs7nvCWUKSQ8/XVUgQUehoK537I= Original-Received: by 10.100.251.5 with SMTP id y5mr4431250anh.125.1216315397797; Thu, 17 Jul 2008 10:23:17 -0700 (PDT) Original-Received: from ?10.0.1.200? ( [97.73.30.9]) by mx.google.com with ESMTPS id i6sm213541wxd.4.2008.07.17.10.23.09 (version=TLSv1/SSLv3 cipher=RC4-MD5); Thu, 17 Jul 2008 10:23:16 -0700 (PDT) In-Reply-To: <200807170324.m6H3OwUW018599@sallyv1.ics.uci.edu> X-Mailer: Apple Mail (2.926) X-detected-kernel: by monty-python.gnu.org: Linux 2.6 (newer, 2) X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.devel:100891 Archived-At: On Jul 16, 2008, at 11:24 PM, Dan Nicolaescu wrote: > Adrian Robert writes: > >> On Jul 16, 2008, at 5:25 AM, Dan Nicolaescu wrote: >> >> >> Index: lisp/startup.el >> >> =================================================================== >> RCS file: /sources/emacs/emacs/lisp/startup.el,v >> retrieving revision 1.494 >> diff -a -u -r1.494 startup.el >> --- lisp/startup.el 2 Jul 2008 01:49:01 -0000 1.494 >> +++ lisp/startup.el 15 Jul 2008 16:54:18 -0000 >> @@ -182,3 +182,6 @@ >> and VALUE is the value which is given to that frame parameter >> \(most options use the argument for this, so VALUE is not >> present).") >> >> +(defconst command-line-ns-option-alist >> + '(("-NSAutoLaunch" 1 ns-ignore-1-arg) >> + ("-NXAutoLaunch" 1 ns-ignore-1-arg) >> [snip] >> >> Can this be put somewhere else? It would be better if all other >> platforms >> do not have to load this definition. >> >> + ;; Add the long NS options to longopts. >> + (setq tem command-line-ns-option-alist) >> + (while tem >> + (if (string-match "^--" (car (car tem))) >> + (setq longopts (cons (list (car (car tem))) longopts))) >> + (setq tem (cdr tem))) >> >> Can this be avoided and use the generic code for command line >> processing? >> >> >> >> NS has followed the X GUI in both these cases, using ns- prefix to >> distinguish >> variables for option lists, etc. that are specific to the NS >> platform, as X- is >> used to indicate X-windows. I do realize that w32 and mac (Carbon) >> seem to >> deal with their arguments in ways not involving this file. The >> question is, >> should every platform be done in the same way as X, or should X and >> NS be >> changed to whatever mac and w32 are doing? > > NS should be following whatever X, w32 and mac are doing. That makes > maintaining the code easier. > > For example there are quite a few functions/variables that X, w32 and > mac call x-BLAH. But ns calls them ns-BLAH. Please rename them to > x-BLAH. Sorry if I was unclear, there are two points here. First, NS has followed X, but mac and w32 do their own (different) thing for arg handling. Things should be consistent, but don't know which ones should be changed. The second point is about naming. I feel that ports that used the "x-" prefix for their own functions did the wrong thing in general, and make things more confusing. Now there is no way to know, from a function name, whether it is implemented generic window system code, non-X port-specific code, or X windows-specific code. It also makes grepping and using automated tools like find-tag difficult to use with emacs source. However using x- for everything makes other things easier (no need for aliasing, etc.), and that is the way things have gone. But in this case, there would be a naming conflict, unless adding some 'if' structure to set command-line-x-option-alist, etc. differently depending on platform (which, is it known at this stage)? >> +#ifdef HAVE_NS >> +abbrev.o buffer.o callint.o cmds.o dispnew.o editfns.o fileio.o >> frame.o \ >> + fontset.o indent.o insdel.o keyboard.o macros.o minibuf.o >> msdos.o >> process.o \ >> + scroll.o sysdep.o term.o widget.o window.o xdisp.o xfaces.o >> xfns.o \ >> + xterm.o xselect.o sound.o: nsgui.h >> +nsfns.o: nsfns.m charset.h nsterm.h nsgui.h frame.h window.h >> buffer.h \ >> + dispextern.h nsgui.h fontset.h $(INTERVAL_SRC) keyboard.h >> blockinput.h \ >> + atimer.h systime.h epaths.h termhooks.h coding.h systime.h $ >> (config_h) >> +nsmenu.o: nsmenu.m termhooks.h frame.h window.h dispextern.h \ >> + nsgui.h keyboard.h blockinput.h atimer.h systime.h buffer.h \ >> + nsterm.h $(config_h) >> +nsterm.o: nsterm.m blockinput.h atimer.h systime.h syssignal.h >> nsterm.h \ >> + nsgui.h frame.h charset.h ccl.h dispextern.h fontset.h >> termhooks.h \ >> + termopts.h termchar.h disptab.h buffer.h window.h keyboard.h \ >> + $(INTERVAL_SRC) process.h coding.h $(config_h) >> +nsselect.o: nsselect.m blockinput.h nsterm.h nsgui.h frame.h $ >> (config_h) >> +nsimage.o: nsimage.m nsterm.h >> +nsfont.o: nsterm.h dispextern.h frame.h lisp.h $(config_h) >> >> Better make this unconditional. >> >> >> Here I aped what the Carbon port does just above these lines. If >> it's wrong, >> which example should I follow? > > Dependencies do not hurt if they are in the generic code, better keep > the Makefile simpler (it is way too complex as it is) OK, so remove both the HAVE_NS and HAVE_CARBON ifdefs for their source dependencies? >> Index: src/frame.c >> ... >> Fselect_window (XFRAME (frame)->selected_window, Qnil); >> >> +#ifdef NS_IMPL_COCOA >> + /* term gets no other notification of this */ >> + if (for_deletion) >> + Fraise_frame(Qnil); >> +#endif >> >> Why isn't his needed for other platforms too? >> >> >> I don't know. I would be happy to get rid of it if I knew. > > IMHO this needs to be debugged and understood better. When does it > happen? The case being hit was, when a frame was closed, no other frame would get focus. >> +#ifndef HAVE_NS /* PENDING: ensure font attrs change goes >> through */ >> >> Better use "FIXME" instead of "PENDING". >> >> >> Are there other options besides FIXME? I use PENDING to flag >> something that is >> not necessarily a bug or even needing change, but that needs to be >> considered. >> What about TODO? > > XXX, TODO and FIXME are the ones that are in use in the tree. > Anything > that has been in use for a while should be fine. OK, changed. >> Index: src/getloadavg.c >> >> =================================================================== >> RCS file: /sources/emacs/emacs/src/getloadavg.c,v >> retrieving revision 1.53 >> diff -a -u -r1.53 getloadavg.c >> --- src/getloadavg.c 8 Jan 2008 20:44:33 -0000 1.53 >> +++ src/getloadavg.c 15 Jul 2008 16:59:32 -0000 >> >> This file comes from gnulib, we try not to change it here. It >> should go >> there first. >> Why wasn't this needed until now? This should not have anything >> to do with >> NS... >> >> >> I don't know, probably it was never working. I actually am not >> sure if it is >> now, but anyway the original is using #ifdef NeXT to check whether >> mach.h is in >> a subdirectory, but it seems all NeXT-derived systems put it in the >> same place >> (e.g., my OS X 10.5.4 machine here). >> >> ACTUALLY, it looks like this file is not even used anymore (grep thru >> Makefile.in), so probably it should just be removed. > > If you don't have a getloadavg.o in your tree, then it's safe to undo > these changes... I don't, but neither does anybody else. ;) How about just removing the .c file? >> Index: src/lisp.h >> >> =================================================================== >> RCS file: /sources/emacs/emacs/src/lisp.h,v >> retrieving revision 1.631 >> diff -a -u -r1.631 lisp.h >> --- src/lisp.h 11 Jul 2008 14:20:06 -0000 1.631 >> +++ src/lisp.h 15 Jul 2008 17:01:36 -0000 >> @@ -28,3 +28,7 @@ >> #define P_(proto) () >> #endif >> >> +#ifdef NS_IMPL_GNUSTEP >> +/* This conflicts with functions in the GNUstep libraries. */ >> +#define hash_remove emacs_hash_remove >> +#endif /* NS_IMPL_GNUSTEP */ >> >> Sounds odd, but if this is indeed true, better rename the >> function in >> emacs to avoid extra #ifdefs. >> >> >> Yes, hash_put, hash_remove, etc. are all used in those libs. Only >> hash_remove >> is declared so publicly within emacs though. > > It looks like hash_remove is only used in fns.c, so better rename it > there, and it can even be made static (BTW nsgui.h also seems to have > this #define). Will do. At one point it was used in >1 c file (else I would have done the static thing) but can't find ChangeLog when that changed. Maybe I was hallucinating again... ;) >> Index: src/terminfo.c >> >> =================================================================== >> RCS file: /sources/emacs/emacs/src/terminfo.c,v >> retrieving revision 1.24 >> diff -a -u -r1.24 terminfo.c >> --- src/terminfo.c 14 May 2008 07:49:52 -0000 1.24 >> +++ src/terminfo.c 15 Jul 2008 17:03:07 -0000 >> @@ -24,4 +24,7 @@ >> so that we do not need to conditionalize the places in Emacs >> that set them. */ >> >> +/* Causes a conflict on OS X 10.3 .*/ >> +#ifndef NS_IMPL_COCOA >> char *UP, *BC, PC; >> +#endif >> >> Does "emacs -nw" work after doing this? How come this wasn't a >> problem >> with the Carbon port? >> >> >> Yes, and I don't know. It's possible something is different in the >> includes >> brought in by Carbon vs. Cocoa. > > Is OS X 10.3 something we still want to support? > If yes, then maybe better rename those variables. As far as Emacs.app, no reason not to support 10.3 for now, but eventually some code can be removed if the decision is to drop it. In truth, I haven't tested it in a while. I don't understand enough about the use of these variables and their relation to the same ones in ncurses includes to want to mess any further with these variables though.