Re: [Emacs-diffs] Changes to emacs/src/emacs.c,v

Re: [Emacs-diffs] Changes to emacs/src/emacs.c,v

[Emanuele Giaquinta]

On Sun, Jul 27, 2008 at 06:25:06PM +0000, Dan Nicolaescu wrote:

> CVSROOT:	/cvsroot/emacs
> Module name:	emacs
> Changes by:	Dan Nicolaescu <dann>	08/07/27 18:24:49
> 
> Index: src/emacs.c
> ===================================================================
> RCS file: /cvsroot/emacs/emacs/src/emacs.c,v
> retrieving revision 1.437
> retrieving revision 1.438
> diff -u -b -r1.437 -r1.438
> --- src/emacs.c	23 Jul 2008 23:37:17 -0000	1.437
> +++ src/emacs.c	27 Jul 2008 18:24:40 -0000	1.438
> @@ -837,7 +837,7 @@
>      run_time_remap (argv[0]);
>  #endif
>  
> -#if defined (MAC_OSX) || defined (NS_IMPL_COCOA)
> +#if defined (NS_IMPL_COCOA)
>    if (!initialized)
>      unexec_init_emacs_zone ();
>  #endif

This hunk is wrong and breaks malloc on os x when building without
toolkit. The reason to have two macros, MAC_OSX and HAVE_CARBON, was to
distinguish bare os x support from carbon gui AFAIU. I didn't check if
there are similar changes to revert as well.

Emanuele

Re: [Emacs-diffs] Changes to emacs/src/emacs.c,v

[Dan Nicolaescu]

Emanuele Giaquinta <emanuele.giaquinta@gmail.com> writes:

  > On Sun, Jul 27, 2008 at 06:25:06PM +0000, Dan Nicolaescu wrote:
  > 
  > > CVSROOT:	/cvsroot/emacs
  > > Module name:	emacs
  > > Changes by:	Dan Nicolaescu <dann>	08/07/27 18:24:49
  > > 
  > > Index: src/emacs.c
  > > ===================================================================
  > > RCS file: /cvsroot/emacs/emacs/src/emacs.c,v
  > > retrieving revision 1.437
  > > retrieving revision 1.438
  > > diff -u -b -r1.437 -r1.438
  > > --- src/emacs.c	23 Jul 2008 23:37:17 -0000	1.437
  > > +++ src/emacs.c	27 Jul 2008 18:24:40 -0000	1.438
  > > @@ -837,7 +837,7 @@
  > >      run_time_remap (argv[0]);
  > >  #endif
  > >  
  > > -#if defined (MAC_OSX) || defined (NS_IMPL_COCOA)
  > > +#if defined (NS_IMPL_COCOA)
  > >    if (!initialized)
  > >      unexec_init_emacs_zone ();
  > >  #endif
  > 
  > This hunk is wrong and breaks malloc on os x when building without
  > toolkit. The reason to have two macros, MAC_OSX and HAVE_CARBON, was to
  > distinguish bare os x support from carbon gui AFAIU.

Unfortunately that is not what the code in configure.in said:

/* TODO: These are used for the Carbon port only. */
#undef MAC_OS
#undef MAC_OSX

What is the right conditional there?

Re: [Emacs-diffs] Changes to emacs/src/emacs.c,v

[Emanuele Giaquinta]

On Mon, Jul 28, 2008 at 03:59:21PM -0700, Dan Nicolaescu wrote:

> Unfortunately that is not what the code in configure.in said:
> 
> /* TODO: These are used for the Carbon port only. */
> #undef MAC_OS
> #undef MAC_OSX
> 
> What is the right conditional there?

I think the right conditional is to check only MAC_OSX (defined in
s/darwin.h), that code is required by unexmacosx.c.

Emanuele

Re: [Emacs-diffs] Changes to emacs/src/emacs.c,v

[Dan Nicolaescu]

Emanuele Giaquinta <emanuele.giaquinta@gmail.com> writes:

  > On Mon, Jul 28, 2008 at 03:59:21PM -0700, Dan Nicolaescu wrote:
  > 
  > > Unfortunately that is not what the code in configure.in said:
  > > 
  > > /* TODO: These are used for the Carbon port only. */
  > > #undef MAC_OS
  > > #undef MAC_OSX
  > > 
  > > What is the right conditional there?
  > 
  > I think the right conditional is to check only MAC_OSX (defined in
  > s/darwin.h), that code is required by unexmacosx.c.

MAC_OSX is only define if !HAVE_NS, so that wouldn't be quite right
either. 

Re: [Emacs-diffs] Changes to emacs/src/emacs.c,v

[Stefan Monnier]

>> This hunk is wrong and breaks malloc on os x when building without
>> toolkit. The reason to have two macros, MAC_OSX and HAVE_CARBON, was to
>> distinguish bare os x support from carbon gui AFAIU.

> Unfortunately that is not what the code in configure.in said:

But that's partly what admin/CPP-DEFINES says.

> What is the right conditional there?

Of course, you can expect that some of those macros aren't always used
100% correctly either.  Which is why a good CPP-DEFINES file is
important, in order to document the intended use, so as to know which
uses are correct and which aren't.


        Stefan

Re: [Emacs-diffs] Changes to emacs/src/emacs.c,v

[Dan Nicolaescu]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

  > >> This hunk is wrong and breaks malloc on os x when building without
  > >> toolkit. The reason to have two macros, MAC_OSX and HAVE_CARBON, was to
  > >> distinguish bare os x support from carbon gui AFAIU.
  > 
  > > Unfortunately that is not what the code in configure.in said:
  > 
  > But that's partly what admin/CPP-DEFINES says.

The configure.in info was the most recent addition, so it looked like
the most reliable source of information.

But that code never worked correctly, puttin #undefine inside an
AH_BOTTOM() has no effect, the code will be output commented out.  
(There are some more instances of using #undef in configure.in ... )
So the NS code was clearly never tested with MAC_OSX undefined, so it
won't be too surprising if there are more problems  of the same kind.

Re: [Emacs-diffs] Changes to emacs/src/emacs.c,v

[Emanuele Giaquinta]

On Mon, Jul 28, 2008 at 04:28:38PM -0700, Dan Nicolaescu wrote:

> Emanuele Giaquinta <emanuele.giaquinta@gmail.com> writes:
> 
>   > On Mon, Jul 28, 2008 at 03:59:21PM -0700, Dan Nicolaescu wrote:
>   > 
>   > > Unfortunately that is not what the code in configure.in said:
>   > > 
>   > > /* TODO: These are used for the Carbon port only. */
>   > > #undef MAC_OS
>   > > #undef MAC_OSX
>   > > 
>   > > What is the right conditional there?
>   > 
>   > I think the right conditional is to check only MAC_OSX (defined in
>   > s/darwin.h), that code is required by unexmacosx.c.
> 
> MAC_OSX is only define if !HAVE_NS, so that wouldn't be quite right
> either. 

Ah, I see. The previous conditional was correct then.

Emanuele

Re: [Emacs-diffs] Changes to emacs/src/emacs.c,v

[Dan Nicolaescu]

Emanuele Giaquinta <emanuele.giaquinta@gmail.com> writes:

  > On Mon, Jul 28, 2008 at 04:28:38PM -0700, Dan Nicolaescu wrote:
  > 
  > > Emanuele Giaquinta <emanuele.giaquinta@gmail.com> writes:
  > > 
  > >   > On Mon, Jul 28, 2008 at 03:59:21PM -0700, Dan Nicolaescu wrote:
  > >   > 
  > >   > > Unfortunately that is not what the code in configure.in said:
  > >   > > 
  > >   > > /* TODO: These are used for the Carbon port only. */
  > >   > > #undef MAC_OS
  > >   > > #undef MAC_OSX
  > >   > > 
  > >   > > What is the right conditional there?
  > >   > 
  > >   > I think the right conditional is to check only MAC_OSX (defined in
  > >   > s/darwin.h), that code is required by unexmacosx.c.
  > > 
  > > MAC_OSX is only define if !HAVE_NS, so that wouldn't be quite right
  > > either. 
  > 
  > Ah, I see. The previous conditional was correct then.

It would make this particular code work, but it might not be enough.
src/s/darwin.h should not be using -DMAC_OSX, it should put that in a
#define, that is the point of darwin.h, to contain #defines.  The macro
should be documented properly in admin/CPP-DEFINES.  Hopefully a mac
person can take care of this.

Re: [Emacs-diffs] Changes to emacs/src/emacs.c,v

[Emanuele Giaquinta]

On Tue, Jul 29, 2008 at 06:13:30AM -0700, Dan Nicolaescu wrote:

> Emanuele Giaquinta <emanuele.giaquinta@gmail.com> writes:
> 
>   > On Mon, Jul 28, 2008 at 04:28:38PM -0700, Dan Nicolaescu wrote:
>   > 
>   > > Emanuele Giaquinta <emanuele.giaquinta@gmail.com> writes:
>   > > 
>   > >   > On Mon, Jul 28, 2008 at 03:59:21PM -0700, Dan Nicolaescu wrote:
>   > >   > 
>   > >   > > Unfortunately that is not what the code in configure.in said:
>   > >   > > 
>   > >   > > /* TODO: These are used for the Carbon port only. */
>   > >   > > #undef MAC_OS
>   > >   > > #undef MAC_OSX
>   > >   > > 
>   > >   > > What is the right conditional there?
>   > >   > 
>   > >   > I think the right conditional is to check only MAC_OSX (defined in
>   > >   > s/darwin.h), that code is required by unexmacosx.c.
>   > > 
>   > > MAC_OSX is only define if !HAVE_NS, so that wouldn't be quite right
>   > > either. 
>   > 
>   > Ah, I see. The previous conditional was correct then.
> 
> It would make this particular code work, but it might not be enough.
> src/s/darwin.h should not be using -DMAC_OSX, it should put that in a
> #define, that is the point of darwin.h, to contain #defines.  The macro
> should be documented properly in admin/CPP-DEFINES.  Hopefully a mac
> person can take care of this.

While I agree that using a #define rather than -D would be nicer, I do
not see what it would change wrt this issue.
MAC_OSX was documented in CPP-DEFINES, maybe the description could have
been better but I do not see what is wrong in reverting that hunk
that is not related to the carbon gui removal and breaks non toolkit
builds, the whole thing can be improved later if needed.

Emanuele

Re: [Emacs-diffs] Changes to emacs/src/emacs.c,v

[Dan Nicolaescu]

Emanuele Giaquinta <emanuele.giaquinta@gmail.com> writes:

  > On Tue, Jul 29, 2008 at 06:13:30AM -0700, Dan Nicolaescu wrote:
  > 
  > > Emanuele Giaquinta <emanuele.giaquinta@gmail.com> writes:
  > > 
  > >   > On Mon, Jul 28, 2008 at 04:28:38PM -0700, Dan Nicolaescu wrote:
  > >   > 
  > >   > > Emanuele Giaquinta <emanuele.giaquinta@gmail.com> writes:
  > >   > > 
  > >   > >   > On Mon, Jul 28, 2008 at 03:59:21PM -0700, Dan Nicolaescu wrote:
  > >   > >   > 
  > >   > >   > > Unfortunately that is not what the code in configure.in said:
  > >   > >   > > 
  > >   > >   > > /* TODO: These are used for the Carbon port only. */
  > >   > >   > > #undef MAC_OS
  > >   > >   > > #undef MAC_OSX
  > >   > >   > > 
  > >   > >   > > What is the right conditional there?
  > >   > >   > 
  > >   > >   > I think the right conditional is to check only MAC_OSX (defined in
  > >   > >   > s/darwin.h), that code is required by unexmacosx.c.
  > >   > > 
  > >   > > MAC_OSX is only define if !HAVE_NS, so that wouldn't be quite right
  > >   > > either. 
  > >   > 
  > >   > Ah, I see. The previous conditional was correct then.
  > > 
  > > It would make this particular code work, but it might not be enough.
  > > src/s/darwin.h should not be using -DMAC_OSX, it should put that in a
  > > #define, that is the point of darwin.h, to contain #defines.  The macro
  > > should be documented properly in admin/CPP-DEFINES.  Hopefully a mac
  > > person can take care of this.
  > 
  > While I agree that using a #define rather than -D would be nicer, I do
  > not see what it would change wrt this issue.
  > MAC_OSX was documented in CPP-DEFINES, maybe the description could have
  > been better but I do not see what is wrong in reverting that hunk
  > that is not related to the carbon gui removal and breaks non toolkit
  > builds, the whole thing can be improved later if needed.

We are going in circles here.  MAC_OSX was documented in CPP-DEFINES in
one way, the latest code added with the NextStep check in documented it
with a completely different meaning: that is only used in Carbon.  That
is why references to MAC_OSX were completely removed when removing
support for Carbon, with the exception of the code in src/s/darwin.h,
which got a FIXME tag asking if it still needed.

So yes, this needs fixing, but the mac people need to figure out what is
the right way to do it, and add proper documentation to CPP-DEFINES.

Meanwhile you can undo the change in your local tree.