Re: fixing non-NS darwin (was: Re: your emacs/src/keyboard.h change)

Re: fixing non-NS darwin (was: Re: your emacs/src/keyboard.h change)

[Adrian Robert]

On Aug 3, 2008, at 11:59 AM, Dan Nicolaescu wrote:

> This code in emacs.c is not right:
>
> #if defined (NS_IMPL_COCOA)
>  if (!initialized)
>    unexec_init_emacs_zone ();
> #endif
>
> it should be done on all MacOSX configs.

I've been looking at this, and made a diff of before and after remove- 
carbon to see what else might have been affected, since I never  
realized that the undef MAC_OSX wasn't working.


> While at it, can you please get rid of  -DMAC_OSX from src/s/darwin.h?

After some discussion with other developers, I think there will need  
to be some kind of #define there, analogous to WINDOWSNT in s/ms- 
w32.h, GNU_LINUX in s/gnu-linux.h, etc..  MAC_OSX is wrong because it  
could be a non-OS X Darwin system.  DARWIN is wrong because that is  
apparently defined by the system includes and/or compiler on non-OS X  
Darwin (see below).  I don't think just the BSD4_2 that's in darwin.h  
is specific enough.

I am thinking something like DARWIN_BASED_OS?



> And also the -Dtemacs ? (AFAICT just using #ifdef emacs instead
> #ifdef temacs has exactly the same effect)

It looks like the entire C_SWITCH_SYSTEM_TEMACS is unused, but  
LD_SWITCH_SYSTEM_TEMACS *is* used, so perhaps it is kept around for  
completeness?  I'll get rid of the line in darwin.h.


> And please add documentation to admin/CPP-DEFINES for the DARWIN  
> macro.

This is not defined anywhere in emacs, but there was this section in  
an earlier version of darwin.h:

#if 0 /* Don't define DARWIN on Mac OS X because CoreFoundation.h uses
         it to distinguish Mac OS X from bare Darwin.  */
#ifndef DARWIN
#define DARWIN 1
#endif
#endif

Does anyone know where this IS defined?  Also, I've been unable to  
find a version of CoreFoundation.h that makes the check referred to.

Re: fixing non-NS darwin (was: Re: your emacs/src/keyboard.h change)

[YAMAMOTO Mitsuharu]

>>>>> On Sun, 3 Aug 2008 12:41:22 -0400, Adrian Robert <adrian.b.robert@gmail.com> said:

> This is not defined anywhere in emacs, but there was this section in  
> an earlier version of darwin.h:

> #if 0 /* Don't define DARWIN on Mac OS X because CoreFoundation.h uses
>          it to distinguish Mac OS X from bare Darwin.  */
> #ifndef DARWIN
> #define DARWIN 1
> #endif
> #endif

> Does anyone know where this IS defined?  Also, I've been unable to  
> find a version of CoreFoundation.h that makes the check referred to.

It's used in CoreFoundation.h in Mac OS X 10.1 - 10.3.

BTW, why the above comment was removed?  The corresponding ChangeLog
entry only says:

  2008-07-10  Dan Nicolaescu  <dann@ics.uci.edu>

	  * unexec.c:
	  * s/vms.h:
	  * s/usg5-4-2.h:
	  * s/sol2-5.h:
	  * s/freebsd.h:
	  * s/darwin.h: Remove dead code.

The comments like this should be left for future reference so people
may not make the same mistake again.

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

Re: fixing non-NS darwin

[Dan Nicolaescu]

Adrian Robert <adrian.b.robert@gmail.com> writes:

  > On Aug 3, 2008, at 11:59 AM, Dan Nicolaescu wrote:
  > 
  > > While at it, can you please get rid of  -DMAC_OSX from src/s/darwin.h?
  > 
  > After some discussion with other developers, I think there will need
  > to be some kind of #define there, analogous to WINDOWSNT in s/ms- 
  > w32.h, GNU_LINUX in s/gnu-linux.h, etc..  MAC_OSX is wrong because it
  > could be a non-OS X Darwin system.  DARWIN is wrong because that is
  > apparently defined by the system includes and/or compiler on non-OS X
  > Darwin (see below).  I don't think just the BSD4_2 that's in darwin.h
  > is specific enough.
  > 
  > I am thinking something like DARWIN_BASED_OS?

Is there something that the compiler defines by default?  If there is,
it would be better to use that instead.
If not, I'd vote for DARWIN_OS because is shorter.


  > > And please add documentation to admin/CPP-DEFINES for the DARWIN
  > > macro.
  > 
  > This is not defined anywhere in emacs, but there was this section in
  > an earlier version of darwin.h:
  > 
  > #if 0 /* Don't define DARWIN on Mac OS X because CoreFoundation.h uses
  >         it to distinguish Mac OS X from bare Darwin.  */
  > #ifndef DARWIN
  > #define DARWIN 1
  > #endif
  > #endif
  > 
  > Does anyone know where this IS defined?  Also, I've been unable to
  > find a version of CoreFoundation.h that makes the check referred to.

So what should happen to the code that checks for DARWIN ?

Re: fixing non-NS darwin

[Dan Nicolaescu]

YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp> writes:

  > >>>>> On Sun, 3 Aug 2008 12:41:22 -0400, Adrian Robert <adrian.b.robert@gmail.com> said:
  > 
  > > This is not defined anywhere in emacs, but there was this section in  
  > > an earlier version of darwin.h:
  > 
  > > #if 0 /* Don't define DARWIN on Mac OS X because CoreFoundation.h uses
  > >          it to distinguish Mac OS X from bare Darwin.  */
  > > #ifndef DARWIN
  > > #define DARWIN 1
  > > #endif
  > > #endif
  > 
  > > Does anyone know where this IS defined?  Also, I've been unable to  
  > > find a version of CoreFoundation.h that makes the check referred to.
  > 
  > It's used in CoreFoundation.h in Mac OS X 10.1 - 10.3.
  > 
  > BTW, why the above comment was removed? 

Because it looked like dead code, and the description was not something
that made it clear that it was needed as documentation, nor was the
structure of the code.  There is a proper place now to properly document
macros:  admin/CPP-DEFINES.

Re: fixing non-NS darwin

[YAMAMOTO Mitsuharu]

>>>>> On Sun, 03 Aug 2008 18:20:05 -0700, Dan Nicolaescu <dann@ics.uci.edu> said:

>> > This is not defined anywhere in emacs, but there was this section in  
>> > an earlier version of darwin.h:
>> 
>> > #if 0 /* Don't define DARWIN on Mac OS X because CoreFoundation.h uses
>> >          it to distinguish Mac OS X from bare Darwin.  */
>> > #ifndef DARWIN
>> > #define DARWIN 1
>> > #endif
>> > #endif
>> 
>> > Does anyone know where this IS defined?  Also, I've been unable to  
>> > find a version of CoreFoundation.h that makes the check referred to.
>> 
>> It's used in CoreFoundation.h in Mac OS X 10.1 - 10.3.
>> 
>> BTW, why the above comment was removed? 

> Because it looked like dead code, and the description was not something
> that made it clear that it was needed as documentation, nor was the
> structure of the code.

It's quite a common way in the Emacs code to comment out with #if 0
with leaving some explanation about why it is disabled.  Especially
for the case that people might make the same mistake again in future
unconsciously if that part were completely removed.

Cleanup tasks are usually tedious, and thus would be much appreciated
if done carefully and appropriately.  But as they are also inherently
optional, not appreciated if done less carefully or unnecessarily
aggressively as in the case of MULTI_KBOARD.

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

Re: fixing non-NS darwin

[Dan Nicolaescu]

YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp> writes:

  > >>>>> On Sun, 03 Aug 2008 18:20:05 -0700, Dan Nicolaescu <dann@ics.uci.edu> said:
  > 
  > >> > This is not defined anywhere in emacs, but there was this section in  
  > >> > an earlier version of darwin.h:
  > >> 
  > >> > #if 0 /* Don't define DARWIN on Mac OS X because CoreFoundation.h uses
  > >> >          it to distinguish Mac OS X from bare Darwin.  */
  > >> > #ifndef DARWIN
  > >> > #define DARWIN 1
  > >> > #endif
  > >> > #endif
  > >> 
  > >> > Does anyone know where this IS defined?  Also, I've been unable to  
  > >> > find a version of CoreFoundation.h that makes the check referred to.
  > >> 
  > >> It's used in CoreFoundation.h in Mac OS X 10.1 - 10.3.
  > >> 
  > >> BTW, why the above comment was removed? 
  > 
  > > Because it looked like dead code, and the description was not something
  > > that made it clear that it was needed as documentation, nor was the
  > > structure of the code.
  > 
  > It's quite a common way in the Emacs code to comment out with #if 0
  > with leaving some explanation about why it is disabled.  Especially
  > for the case that people might make the same mistake again in future
  > unconsciously if that part were completely removed.

So? Everyone is aware of that.
But that was absolutely not the case in this particular instance.  The
comments and code did not make the intention clear for the casual reader.

If you actually want to contribute something positive, please add the
missing documentation and fix the code.

  > Cleanup tasks are usually tedious, and thus would be much appreciated
  > if done carefully and appropriately.  But as they are also inherently
  > optional, not appreciated if done less carefully or unnecessarily
  > aggressively as in the case of MULTI_KBOARD.

Did you just wait for an occasion to attack me on an unrelated item?  Grow up!

Sorry, I am not interested in your badgering, and given you past
abhorrent behavior you are not in a position to lecture anyone.
Do I have to repeat it yet again?  STOP WAISTING MY TIME!

Re: fixing non-NS darwin

[YAMAMOTO Mitsuharu]

>>>>> On Sun, 03 Aug 2008 19:26:24 -0700, Dan Nicolaescu <dann@ics.uci.edu> said:

>> It's quite a common way in the Emacs code to comment out with #if 0
>> with leaving some explanation about why it is disabled.  Especially
>> for the case that people might make the same mistake again in
>> future unconsciously if that part were completely removed.

> So? Everyone is aware of that.  But that was absolutely not the case
> in this particular instance.  The comments and code did not make the
> intention clear for the casual reader.

Would casual readers care about admin/CPP-DEFINES?  Anyway, I did not
expect casual readers would touch the code in the CVS, even for
cleanups.

> If you actually want to contribute something positive, please add
> the missing documentation and fix the code.

It's not just a local problem.  I'm concerned about possible removals
of other important comments in your past/future cleanups.  It's really
inefficient if every developer has to check the excessive removal of
comments in (inherently optional) cleanups.

>> Cleanup tasks are usually tedious, and thus would be much
>> appreciated if done carefully and appropriately.  But as they are
>> also inherently optional, not appreciated if done less carefully or
>> unnecessarily aggressively as in the case of MULTI_KBOARD.

> Did you just wait for an occasion to attack me on an unrelated item?
> Grow up!

Unrelated?  I'd rather think these troubles come from the same
mindset.  Did I wait?  I hoped such kind of troubles wouldn't happen
again, but actually I could anticipate that would.

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

Re: fixing non-NS darwin

[Dan Nicolaescu]

YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp> writes:

  > >>>>> On Sun, 03 Aug 2008 19:26:24 -0700, Dan Nicolaescu <dann@ics.uci.edu> said:
  > 
  > >> It's quite a common way in the Emacs code to comment out with #if 0
  > >> with leaving some explanation about why it is disabled.  Especially
  > >> for the case that people might make the same mistake again in
  > >> future unconsciously if that part were completely removed.
  > 
  > > So? Everyone is aware of that.  But that was absolutely not the case
  > > in this particular instance.  The comments and code did not make the
  > > intention clear for the casual reader.
  > 
  > Would casual readers care about admin/CPP-DEFINES?  

Of course, that is why it exists.

  > > If you actually want to contribute something positive, please add
  > > the missing documentation and fix the code.
  > 
  > It's not just a local problem.  I'm concerned about possible removals
  > of other important comments in your past/future cleanups.  It's really
  > inefficient if every developer has to check the excessive removal of
  > comments in (inherently optional) cleanups.

As usual you are making a mountain out of a molehill and making a
racket _again_ from nothing.  
Sorry, this is beyond silly.  So again: QUIT WAISTING MY TIME!

Re: fixing non-NS darwin

[Thien-Thi Nguyen]

() Dan Nicolaescu <dann@ics.uci.edu>
() Sun, 03 Aug 2008 21:02:29 -0700

     > Would casual readers care about admin/CPP-DEFINES?  

   Of course, that is why it exists.

   [...]

   So again: QUIT WAISTING MY TIME!

Dan, why don't you DTRT and add a blurb to admin/CPP-DEFINES?
This small effort will save everyone time.

Alternatively, amend the ChangeLog to bring it up to standard.

thi

Re: fixing non-NS darwin

[Dan Nicolaescu]

Thien-Thi Nguyen <ttn@gnuvola.org> writes:

  > () Dan Nicolaescu <dann@ics.uci.edu>
  > () Sun, 03 Aug 2008 21:02:29 -0700
  > 
  >      > Would casual readers care about admin/CPP-DEFINES?  
  > 
  >    Of course, that is why it exists.
  > 
  >    [...]
  > 
  >    So again: QUIT WAISTING MY TIME!
  > 
  > Dan, why don't you DTRT and add a blurb to admin/CPP-DEFINES?
  > This small effort will save everyone time.

This thread started because I didn't know the correct meaning of that
macro, not did the current maintainer...

Re: fixing non-NS darwin (was: Re: your emacs/src/keyboard.h change)

[Adrian Robert]

On Aug 3, 2008, at 11:59 AM, Dan Nicolaescu wrote:

> While at it, can you please get rid of  -DMAC_OSX from src/s/darwin.h?
>
> And also the -Dtemacs ? (AFAICT just using #ifdef emacs instead
> #ifdef temacs has exactly the same effect)

After some investigation, -Demacs is given both when compiling stuff  
in src and movemail.c in lib-src.  However, the NS (and formerly  
Carbon) ports need to distinguish compilation of emacs itself from  
utility programs, to trigger use of special malloc functions in  
unexmacosc.c.  C_SWITCH_SYSTEM_TEMACS and -Dtemacs are used for this  
purpose.

Re: fixing non-NS darwin (was: Re: your emacs/src/keyboard.h change)

[Adrian Robert]

On Aug 3, 2008, at 12:41 PM, Adrian Robert wrote:

>
> On Aug 3, 2008, at 11:59 AM, Dan Nicolaescu wrote:
>
>> This code in emacs.c is not right:
>>
>> #if defined (NS_IMPL_COCOA)
>> if (!initialized)
>>   unexec_init_emacs_zone ();
>> #endif
>>
>> it should be done on all MacOSX configs.
>
> I've been looking at this, and made a diff of before and after  
> remove-carbon to see what else might have been affected, since I  
> never realized that the undef MAC_OSX wasn't working.

OK, the above should be fixed.  (DARWIN_OS is set in darwin.h and used  
to indicate on OS X or pure Darwin; unexmacosx.c should be at some  
point renamed to unexdarwin.c.)

I've also gone through the removal diff and wanted to ask about the  
bit below since it may be something that all terms should do:

Index: atimer.c
===================================================================
RCS file: /sources/emacs/emacs/src/atimer.c,v
retrieving revision 1.28
retrieving revision 1.27
diff -u -r1.28 -r1.27
--- atimer.c	27 Jul 2008 18:24:40 -0000	1.28
+++ atimer.c	14 May 2008 07:49:08 -0000	1.27
@@ -368,7 +368,9 @@

        t = atimers;
        atimers = atimers->next;
+#ifndef MAC_OSX
        t->fn (t);
+#endif

        if (t->type == ATIMER_CONTINUOUS)
  	{
@@ -380,6 +382,10 @@
  	  t->next = free_atimers;
  	  free_atimers = t;
  	}
+#ifdef MAC_OSX
+      /* Fix for Ctrl-G.  Perhaps this should apply to all platforms.  
*/
+      t->fn (t);
+#endif

        EMACS_GET_TIME (now);
      }

Re: fixing non-NS darwin

[Dan Nicolaescu]

Adrian Robert <adrian.b.robert@gmail.com> writes:

  > I've also gone through the removal diff and wanted to ask about the
  > bit below since it may be something that all terms should do:
  > 
  > Index: atimer.c
  > ===================================================================
  > RCS file: /sources/emacs/emacs/src/atimer.c,v
  > retrieving revision 1.28
  > retrieving revision 1.27
  > diff -u -r1.28 -r1.27
  > --- atimer.c	27 Jul 2008 18:24:40 -0000	1.28
  > +++ atimer.c	14 May 2008 07:49:08 -0000	1.27
  > @@ -368,7 +368,9 @@
  > 
  >        t = atimers;
  >        atimers = atimers->next;
  > +#ifndef MAC_OSX
  >        t->fn (t);
  > +#endif
  > 
  >        if (t->type == ATIMER_CONTINUOUS)
  >  	{
  > @@ -380,6 +382,10 @@
  >  	  t->next = free_atimers;
  >  	  free_atimers = t;
  >  	}
  > +#ifdef MAC_OSX
  > +      /* Fix for Ctrl-G.  Perhaps this should apply to all
  > platforms. */
  > +      t->fn (t);
  > +#endif
  > 
  >        EMACS_GET_TIME (now);
  >      }

Is there evidence that this fixes a bug?  Otherwise it would be very
likely against the feature freeze rules to check in something like
this.

Re: fixing non-NS darwin (was: Re: your emacs/src/keyboard.h change)

[YAMAMOTO Mitsuharu]

>>>>> On Mon, 4 Aug 2008 12:11:31 -0400, Adrian Robert <adrian.b.robert@gmail.com> said:

> I've also gone through the removal diff and wanted to ask about the  
> bit below since it may be something that all terms should do:

Well, careful (as opposed to casual) readers would pay attention to
such a comment (not written by me, IIRC).

A related discussion is found at:

  http://lists.gnu.org/archive/html/emacs-pretest-bug/2004-04/msg00141.html

Still I can't clearly explain why it is ok to do a longjmp and not
execute the call to set_alarm from alarm_signal_handler.  But this
change does fix a particular problem and at least does not make things
worse, and it has been working well with Carbon Emacs 22 for a long
time.

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

> Index: atimer.c
> ===================================================================
> RCS file: /sources/emacs/emacs/src/atimer.c,v
> retrieving revision 1.28
> retrieving revision 1.27
> diff -u -r1.28 -r1.27
> --- atimer.c	27 Jul 2008 18:24:40 -0000	1.28
> +++ atimer.c	14 May 2008 07:49:08 -0000	1.27
> @@ -368,7 +368,9 @@
>
>         t = atimers;
>         atimers = atimers->next;
> +#ifndef MAC_OSX
>         t-> fn (t);
> +#endif
>
>         if (t->type == ATIMER_CONTINUOUS)
>   	{
> @@ -380,6 +382,10 @@
>         t->next = free_atimers;
>   	  free_atimers = t;
>   	}
> +#ifdef MAC_OSX
> +      /* Fix for Ctrl-G.  Perhaps this should apply to all platforms.  
> */
> +      t->fn (t);
> +#endif
>
>         EMACS_GET_TIME (now);
>       }