all messages for Emacs-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
* Re: [Emacs-diffs] Changes to emacs/src/emacs.c,v
       [not found] <E1KNAva-0007QI-CF@cvs.savannah.gnu.org>
@ 2008-07-28 22:46 ` Emanuele Giaquinta
  2008-07-28 22:59   ` Dan Nicolaescu
  0 siblings, 1 reply; 10+ messages in thread
From: Emanuele Giaquinta @ 2008-07-28 22:46 UTC (permalink / raw)
  To: Dan Nicolaescu; +Cc: emacs-devel

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




^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Emacs-diffs] Changes to emacs/src/emacs.c,v
  2008-07-28 22:46 ` [Emacs-diffs] Changes to emacs/src/emacs.c,v Emanuele Giaquinta
@ 2008-07-28 22:59   ` Dan Nicolaescu
  2008-07-28 23:18     ` Emanuele Giaquinta
  2008-07-29  2:47     ` Stefan Monnier
  0 siblings, 2 replies; 10+ messages in thread
From: Dan Nicolaescu @ 2008-07-28 22:59 UTC (permalink / raw)
  To: Emanuele Giaquinta; +Cc: emacs-devel

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?




^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Emacs-diffs] Changes to emacs/src/emacs.c,v
  2008-07-28 22:59   ` Dan Nicolaescu
@ 2008-07-28 23:18     ` Emanuele Giaquinta
  2008-07-28 23:28       ` Dan Nicolaescu
  2008-07-29  2:47     ` Stefan Monnier
  1 sibling, 1 reply; 10+ messages in thread
From: Emanuele Giaquinta @ 2008-07-28 23:18 UTC (permalink / raw)
  To: Dan Nicolaescu; +Cc: emacs-devel

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




^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Emacs-diffs] Changes to emacs/src/emacs.c,v
  2008-07-28 23:18     ` Emanuele Giaquinta
@ 2008-07-28 23:28       ` Dan Nicolaescu
  2008-07-29  7:58         ` Emanuele Giaquinta
  0 siblings, 1 reply; 10+ messages in thread
From: Dan Nicolaescu @ 2008-07-28 23:28 UTC (permalink / raw)
  To: Emanuele Giaquinta; +Cc: emacs-devel

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. 




^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Emacs-diffs] Changes to emacs/src/emacs.c,v
  2008-07-28 22:59   ` Dan Nicolaescu
  2008-07-28 23:18     ` Emanuele Giaquinta
@ 2008-07-29  2:47     ` Stefan Monnier
  2008-07-29  4:43       ` Dan Nicolaescu
  1 sibling, 1 reply; 10+ messages in thread
From: Stefan Monnier @ 2008-07-29  2:47 UTC (permalink / raw)
  To: Dan Nicolaescu; +Cc: Emanuele Giaquinta, emacs-devel

>> 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




^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Emacs-diffs] Changes to emacs/src/emacs.c,v
  2008-07-29  2:47     ` Stefan Monnier
@ 2008-07-29  4:43       ` Dan Nicolaescu
  0 siblings, 0 replies; 10+ messages in thread
From: Dan Nicolaescu @ 2008-07-29  4:43 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: Emanuele Giaquinta, emacs-devel

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.




^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Emacs-diffs] Changes to emacs/src/emacs.c,v
  2008-07-28 23:28       ` Dan Nicolaescu
@ 2008-07-29  7:58         ` Emanuele Giaquinta
  2008-07-29 13:13           ` Dan Nicolaescu
  0 siblings, 1 reply; 10+ messages in thread
From: Emanuele Giaquinta @ 2008-07-29  7:58 UTC (permalink / raw)
  To: Dan Nicolaescu; +Cc: emacs-devel

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




^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Emacs-diffs] Changes to emacs/src/emacs.c,v
  2008-07-29  7:58         ` Emanuele Giaquinta
@ 2008-07-29 13:13           ` Dan Nicolaescu
  2008-07-29 13:40             ` Emanuele Giaquinta
  0 siblings, 1 reply; 10+ messages in thread
From: Dan Nicolaescu @ 2008-07-29 13:13 UTC (permalink / raw)
  To: Emanuele Giaquinta; +Cc: emacs-devel

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.




^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Emacs-diffs] Changes to emacs/src/emacs.c,v
  2008-07-29 13:13           ` Dan Nicolaescu
@ 2008-07-29 13:40             ` Emanuele Giaquinta
  2008-07-29 14:00               ` Dan Nicolaescu
  0 siblings, 1 reply; 10+ messages in thread
From: Emanuele Giaquinta @ 2008-07-29 13:40 UTC (permalink / raw)
  To: Dan Nicolaescu; +Cc: emacs-devel

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




^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Emacs-diffs] Changes to emacs/src/emacs.c,v
  2008-07-29 13:40             ` Emanuele Giaquinta
@ 2008-07-29 14:00               ` Dan Nicolaescu
  0 siblings, 0 replies; 10+ messages in thread
From: Dan Nicolaescu @ 2008-07-29 14:00 UTC (permalink / raw)
  To: Emanuele Giaquinta; +Cc: emacs-devel

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.




^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2008-07-29 14:00 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <E1KNAva-0007QI-CF@cvs.savannah.gnu.org>
2008-07-28 22:46 ` [Emacs-diffs] Changes to emacs/src/emacs.c,v Emanuele Giaquinta
2008-07-28 22:59   ` Dan Nicolaescu
2008-07-28 23:18     ` Emanuele Giaquinta
2008-07-28 23:28       ` Dan Nicolaescu
2008-07-29  7:58         ` Emanuele Giaquinta
2008-07-29 13:13           ` Dan Nicolaescu
2008-07-29 13:40             ` Emanuele Giaquinta
2008-07-29 14:00               ` Dan Nicolaescu
2008-07-29  2:47     ` Stefan Monnier
2008-07-29  4:43       ` Dan Nicolaescu

Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/emacs.git
	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.