EMACS_INT cleanup

EMACS_INT cleanup

[Lars Magne Ingebrigtsen]

[-- Attachment #1: Type: text/plain, Size: 229 bytes --]

I went through marker.c and fixed all the EMACS_INT/int problems there.
Before committing, could somebody take a peek and see whether it looks
saneish?

I can't actually test, since the interval cleanup makes Emacs
segfault... 


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: marker.diff --]
[-- Type: text/x-diff, Size: 7809 bytes --]

=== modified file 'src/ChangeLog'
--- src/ChangeLog	2010-09-23 17:10:21 +0000
+++ src/ChangeLog	2010-09-23 18:33:22 +0000
@@ -1,3 +1,14 @@
+2010-09-23  Lars Magne Ingebrigtsen  <larsi@gnus.org>
+
+	* lisp.h: Change the definition of all marker.c functions that
+	take and return buffer stuff to be EMACS_INT instead of int.
+
+	* marker.c (buf_charpos_to_bytepos, CONSIDER, set_marker_both)
+	(buf_charpos_to_bytepos, bytepos_to_charpos)
+	(buf_bytepos_to_charpos, Fbuffer_has_markers_at)
+	(set_marker_restricted, set_marker_both): Convert int to EMACS_INT
+	for all buffer positions.
+
 2010-09-23  Eli Zaretskii  <eliz@gnu.org>
 
 	* editfns.c (transpose_markers, update_buffer_properties)

=== modified file 'src/lisp.h'
--- src/lisp.h	2010-09-23 17:10:21 +0000
+++ src/lisp.h	2010-09-23 18:30:34 +0000
@@ -3054,17 +3054,17 @@
 EXFUN (Fmarker_buffer, 1);
 EXFUN (Fcopy_marker, 2);
 EXFUN (Fset_marker, 3);
-extern int marker_position (Lisp_Object);
-extern int marker_byte_position (Lisp_Object);
+extern EMACS_INT marker_position (Lisp_Object);
+extern EMACS_INT marker_byte_position (Lisp_Object);
 extern void clear_charpos_cache (struct buffer *);
-extern int charpos_to_bytepos (int);
-extern int buf_charpos_to_bytepos (struct buffer *, int);
-extern int buf_bytepos_to_charpos (struct buffer *, int);
+extern EMACS_INT charpos_to_bytepos (EMACS_INT);
+extern EMACS_INT buf_charpos_to_bytepos (struct buffer *, EMACS_INT);
+extern EMACS_INT buf_bytepos_to_charpos (struct buffer *, EMACS_INT);
 extern void unchain_marker (struct Lisp_Marker *marker);
 extern Lisp_Object set_marker_restricted (Lisp_Object, Lisp_Object, Lisp_Object);
-extern Lisp_Object set_marker_both (Lisp_Object, Lisp_Object, int, int);
+extern Lisp_Object set_marker_both (Lisp_Object, Lisp_Object, EMACS_INT, EMACS_INT);
 extern Lisp_Object set_marker_restricted_both (Lisp_Object, Lisp_Object,
-                                               int, int);
+                                               EMACS_INT, EMACS_INT);
 extern void syms_of_marker (void);
 
 /* Defined in fileio.c */

=== modified file 'src/marker.c'
--- src/marker.c	2010-08-30 12:47:49 +0000
+++ src/marker.c	2010-09-23 18:35:58 +0000
@@ -27,12 +27,12 @@
 /* Record one cached position found recently by
    buf_charpos_to_bytepos or buf_bytepos_to_charpos.  */
 
-static int cached_charpos;
-static int cached_bytepos;
+static EMACS_INT cached_charpos;
+static EMACS_INT cached_bytepos;
 static struct buffer *cached_buffer;
 static int cached_modiff;
 
-static void byte_char_debug_check (struct buffer *, int, int);
+static void byte_char_debug_check (struct buffer *, EMACS_INT, EMACS_INT);
 
 /* Nonzero means enable debugging checks on byte/char correspondences.  */
 
@@ -60,12 +60,12 @@
 
 #define CONSIDER(CHARPOS, BYTEPOS)					\
 {									\
-  int this_charpos = (CHARPOS);						\
+  EMACS_INT this_charpos = (CHARPOS);					\
   int changed = 0;							\
 									\
   if (this_charpos == charpos)						\
     {									\
-      int value = (BYTEPOS);						\
+      EMACS_INT value = (BYTEPOS);				       	\
       if (byte_debug_flag)						\
 	byte_char_debug_check (b, charpos, value);			\
       return value;							\
@@ -90,7 +90,7 @@
     {									\
       if (best_above - best_below == best_above_byte - best_below_byte)	\
         {								\
-	  int value = best_below_byte + (charpos - best_below);		\
+	  EMACS_INT value = best_below_byte + (charpos - best_below);	\
 	  if (byte_debug_flag)						\
 	    byte_char_debug_check (b, charpos, value);			\
 	  return value;							\
@@ -99,9 +99,9 @@
 }
 
 static void
-byte_char_debug_check (struct buffer *b, int charpos, int bytepos)
+byte_char_debug_check (struct buffer *b, EMACS_INT charpos, EMACS_INT bytepos)
 {
-  int nchars = 0;
+  EMACS_INT nchars = 0;
 
   if (bytepos > BUF_GPT_BYTE (b))
     {
@@ -118,18 +118,18 @@
     abort ();
 }
 
-int
-charpos_to_bytepos (int charpos)
+EMACS_INT
+charpos_to_bytepos (EMACS_INT charpos)
 {
   return buf_charpos_to_bytepos (current_buffer, charpos);
 }
 
-int
-buf_charpos_to_bytepos (struct buffer *b, int charpos)
+EMACS_INT
+buf_charpos_to_bytepos (struct buffer *b, EMACS_INT charpos)
 {
   struct Lisp_Marker *tail;
-  int best_above, best_above_byte;
-  int best_below, best_below_byte;
+  EMACS_INT best_above, best_above_byte;
+  EMACS_INT best_below, best_below_byte;
 
   if (charpos < BUF_BEG (b) || charpos > BUF_Z (b))
     abort ();
@@ -269,12 +269,12 @@
 
 #define CONSIDER(BYTEPOS, CHARPOS)					\
 {									\
-  int this_bytepos = (BYTEPOS);						\
+  EMACS_INT this_bytepos = (BYTEPOS);					\
   int changed = 0;							\
 									\
   if (this_bytepos == bytepos)						\
     {									\
-      int value = (CHARPOS);						\
+      EMACS_INT value = (CHARPOS);				       	\
       if (byte_debug_flag)						\
 	byte_char_debug_check (b, value, bytepos);			\
       return value;							\
@@ -299,7 +299,7 @@
     {									\
       if (best_above - best_below == best_above_byte - best_below_byte)	\
 	{								\
-	  int value = best_below + (bytepos - best_below_byte);		\
+	  EMACS_INT value = best_below + (bytepos - best_below_byte);	\
 	  if (byte_debug_flag)						\
 	    byte_char_debug_check (b, value, bytepos);			\
 	  return value;							\
@@ -307,18 +307,18 @@
     }									\
 }
 
-int
-bytepos_to_charpos (int bytepos)
+EMACS_INT
+bytepos_to_charpos (EMACS_INT bytepos)
 {
   return buf_bytepos_to_charpos (current_buffer, bytepos);
 }
 
-int
-buf_bytepos_to_charpos (struct buffer *b, int bytepos)
+EMACS_INT
+buf_bytepos_to_charpos (struct buffer *b, EMACS_INT bytepos)
 {
   struct Lisp_Marker *tail;
-  int best_above, best_above_byte;
-  int best_below, best_below_byte;
+  EMACS_INT best_above, best_above_byte;
+  EMACS_INT best_below, best_below_byte;
 
   if (bytepos < BUF_BEG_BYTE (b) || bytepos > BUF_Z_BYTE (b))
     abort ();
@@ -470,7 +470,7 @@
 Returns MARKER.  */)
   (Lisp_Object marker, Lisp_Object position, Lisp_Object buffer)
 {
-  register int charno, bytepos;
+  register EMACS_INT charno, bytepos;
   register struct buffer *b;
   register struct Lisp_Marker *m;
 
@@ -545,7 +545,7 @@
 Lisp_Object
 set_marker_restricted (Lisp_Object marker, Lisp_Object pos, Lisp_Object buffer)
 {
-  register int charno, bytepos;
+  register EMACS_INT charno, bytepos;
   register struct buffer *b;
   register struct Lisp_Marker *m;
 
@@ -618,7 +618,7 @@
    character position and the corresponding byte position.  */
 
 Lisp_Object
-set_marker_both (Lisp_Object marker, Lisp_Object buffer, int charpos, int bytepos)
+set_marker_both (Lisp_Object marker, Lisp_Object buffer, EMACS_INT charpos, EMACS_INT bytepos)
 {
   register struct buffer *b;
   register struct Lisp_Marker *m;
@@ -666,7 +666,7 @@
    be outside the visible part.  */
 
 Lisp_Object
-set_marker_restricted_both (Lisp_Object marker, Lisp_Object buffer, int charpos, int bytepos)
+set_marker_restricted_both (Lisp_Object marker, Lisp_Object buffer, EMACS_INT charpos, EMACS_INT bytepos)
 {
   register struct buffer *b;
   register struct Lisp_Marker *m;
@@ -776,7 +776,7 @@
 
 /* Return the char position of marker MARKER, as a C integer.  */
 
-int
+EMACS_INT
 marker_position (Lisp_Object marker)
 {
   register struct Lisp_Marker *m = XMARKER (marker);
@@ -790,12 +790,12 @@
 
 /* Return the byte position of marker MARKER, as a C integer.  */
 
-int
+EMACS_INT
 marker_byte_position (Lisp_Object marker)
 {
   register struct Lisp_Marker *m = XMARKER (marker);
   register struct buffer *buf = m->buffer;
-  register int i = m->bytepos;
+  register EMACS_INT i = m->bytepos;
 
   if (!buf)
     error ("Marker does not point anywhere");
@@ -856,7 +856,7 @@
   (Lisp_Object position)
 {
   register struct Lisp_Marker *tail;
-  register int charno;
+  register EMACS_INT charno;
 
   charno = XINT (position);
 


[-- Attachment #3: Type: text/plain, Size: 103 bytes --]


-- 
(domestic pets only, the antidote for overdose, milk.)
  larsi@gnus.org * Lars Magne Ingebrigtsen

Re: EMACS_INT cleanup

[Eli Zaretskii]

> From: Lars Magne Ingebrigtsen <larsi@gnus.org>
> Date: Thu, 23 Sep 2010 20:37:45 +0200
> 
> I went through marker.c and fixed all the EMACS_INT/int problems there.
> Before committing, could somebody take a peek and see whether it looks
> saneish?

Looks good, thanks.

> I can't actually test, since the interval cleanup makes Emacs
> segfault... 

Should be fixed by now, sorry.

Re: EMACS_INT cleanup

[Lars Magne Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> I went through marker.c and fixed all the EMACS_INT/int problems there.
>> Before committing, could somebody take a peek and see whether it looks
>> saneish?
>
> Looks good, thanks.

It compiles and doesn't segfault, so I'm checking it in.

-- 
(domestic pets only, the antidote for overdose, milk.)
  larsi@gnus.org * Lars Magne Ingebrigtsen

Re: EMACS_INT cleanup

[Lars Magne Ingebrigtsen]

I'm doing doc.c now, and it does:

	  sym = oblookup (Vobarray, p + 2,
			  multibyte_chars_in_text (p + 2, end - p - 2),
			  end - p - 2);

which is:

Lisp_Object
oblookup (Lisp_Object obarray, register const char *ptr, int size, int size_byte)

The last parameter there is an EMACS_INT.  Should I change oblookup to
take EMACS_INT parameters, in case we want to intern a 2.1GB long
string? 

-- 
(domestic pets only, the antidote for overdose, milk.)
  larsi@gnus.org * Lars Magne Ingebrigtsen

Re: EMACS_INT cleanup

[Lars Magne Ingebrigtsen]

Lars Magne Ingebrigtsen <larsi@gnus.org> writes:

> Lisp_Object
> oblookup (Lisp_Object obarray, register const char *ptr, int size, int size_byte)
>
> The last parameter there is an EMACS_INT.  Should I change oblookup to
> take EMACS_INT parameters, in case we want to intern a 2.1GB long
> string? 

I mean, it sounds nonsensical, but 1) it makes it compile without any
other warnings, and 2) if you've said (setq a (buffer-string)) on a big
buffer and then happen to (intern a), you don't want a segfault, do you?

-- 
(domestic pets only, the antidote for overdose, milk.)
  larsi@gnus.org * Lars Magne Ingebrigtsen

Re: EMACS_INT cleanup

[Eli Zaretskii]

> From: Lars Magne Ingebrigtsen <larsi@gnus.org>
> Date: Thu, 23 Sep 2010 21:34:29 +0200
> 
> Lars Magne Ingebrigtsen <larsi@gnus.org> writes:
> 
> > Lisp_Object
> > oblookup (Lisp_Object obarray, register const char *ptr, int size, int size_byte)
> >
> > The last parameter there is an EMACS_INT.  Should I change oblookup to
> > take EMACS_INT parameters, in case we want to intern a 2.1GB long
> > string? 
> 
> I mean, it sounds nonsensical, but 1) it makes it compile without any
> other warnings, and 2) if you've said (setq a (buffer-string)) on a big
> buffer and then happen to (intern a), you don't want a segfault, do you?

I see no harm in having both size and size_byte be EMACS_INT.

Re: EMACS_INT cleanup

[Lars Magne Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> I see no harm in having both size and size_byte be EMACS_INT.

Ok; I'll convert it.

The one thing that gives the most warnings is USE_SAFE_ALLOCA.  It's a
bit too obscure to me to make out what it's actually doing.  A cast
somewhere in the macro would probably do the trick, but where?

-- 
(domestic pets only, the antidote for overdose, milk.)
  larsi@gnus.org * Lars Magne Ingebrigtsen

Re: EMACS_INT cleanup

[David Kastrup]

Lars Magne Ingebrigtsen <larsi@gnus.org> writes:

> I mean, it sounds nonsensical, but 1) it makes it compile without any
> other warnings, and 2) if you've said (setq a (buffer-string)) on a
> big buffer and then happen to (intern a), you don't want a segfault,
> do you?

It might distract you enough as to not make your getaway before the
memory bloat police arrives and catches you red-black-handed.

-- 
David Kastrup

Re: EMACS_INT cleanup

[Lars Magne Ingebrigtsen]

David Kastrup <dak@gnu.org> writes:

>> I mean, it sounds nonsensical, but 1) it makes it compile without any
>> other warnings, and 2) if you've said (setq a (buffer-string)) on a
>> big buffer and then happen to (intern a), you don't want a segfault,
>> do you?
>
> It might distract you enough as to not make your getaway before the
> memory bloat police arrives and catches you red-black-handed.

Yeah, and then if you unintern nil, and then rm -rf /, then you're
really in trouble.

-- 
(domestic pets only, the antidote for overdose, milk.)
  larsi@gnus.org * Lars Magne Ingebrigtsen

Re: EMACS_INT cleanup

[Eli Zaretskii]

> From: Lars Magne Ingebrigtsen <larsi@gnus.org>
> Date: Thu, 23 Sep 2010 22:05:16 +0200
> 
> The one thing that gives the most warnings is USE_SAFE_ALLOCA.  It's a
> bit too obscure to me to make out what it's actually doing.

It defines variables used to free xmalloc'ed buffer when it goes out
of scope.

> A cast somewhere in the macro would probably do the trick, but
> where?

Does it help to make sa_count's type ptrdiff_t instead of int?

Re: EMACS_INT cleanup

[Lars Magne Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> A cast somewhere in the macro would probably do the trick, but
>> where?
>
> Does it help to make sa_count's type ptrdiff_t instead of int?

I just added the (int) here, which seemed to do the trick:

#define USE_SAFE_ALLOCA			\
  int sa_count = (int) SPECPDL_INDEX (), sa_must_free = 0


-- 
(domestic pets only, the antidote for overdose, milk.)
  larsi@gnus.org * Lars Magne Ingebrigtsen

Re: EMACS_INT cleanup

[Eli Zaretskii]

> From: Lars Magne Ingebrigtsen <larsi@gnus.org>
> Date: Thu, 23 Sep 2010 22:29:49 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> A cast somewhere in the macro would probably do the trick, but
> >> where?
> >
> > Does it help to make sa_count's type ptrdiff_t instead of int?
> 
> I just added the (int) here, which seemed to do the trick:
> 
> #define USE_SAFE_ALLOCA			\
>   int sa_count = (int) SPECPDL_INDEX (), sa_must_free = 0

If ptrdiff_t works, it is a better solution, because SPECPDL_INDEX is
defined like this:

    extern struct specbinding *specpdl;
    extern struct specbinding *specpdl_ptr;
    ...
    #define SPECPDL_INDEX()	(specpdl_ptr - specpdl)

So it returns a difference between two pointers, which is exactly what
ptrdiff_t is for (as its name hints).

Casting it to an int will DTWT on a 64-bit host.

sa_must_free can stay an int, btw.

Re: EMACS_INT cleanup

[Lars Magne Ingebrigtsen]

Lars Magne Ingebrigtsen <larsi@gnus.org> writes:

> I just added the (int) here, which seemed to do the trick:
>
> #define USE_SAFE_ALLOCA			\
>   int sa_count = (int) SPECPDL_INDEX (), sa_must_free = 0

Or perhaps this would be better:

#define SPECPDL_INDEX()	((int) (specpdl_ptr - specpdl))

It'd get rid of a lot of warnings, and we're really returning an int,
aren't we?  Sort of?

-- 
(domestic pets only, the antidote for overdose, milk.)
  larsi@gnus.org * Lars Magne Ingebrigtsen

Re: EMACS_INT cleanup

[Eli Zaretskii]

> From: Lars Magne Ingebrigtsen <larsi@gnus.org>
> Date: Thu, 23 Sep 2010 22:41:45 +0200
> 
> Lars Magne Ingebrigtsen <larsi@gnus.org> writes:
> 
> > I just added the (int) here, which seemed to do the trick:
> >
> > #define USE_SAFE_ALLOCA			\
> >   int sa_count = (int) SPECPDL_INDEX (), sa_must_free = 0
> 
> Or perhaps this would be better:
> 
> #define SPECPDL_INDEX()	((int) (specpdl_ptr - specpdl))
> 
> It'd get rid of a lot of warnings, and we're really returning an int,
> aren't we?  Sort of?

Better make it a type that is 64-bit wide on a 64-bit host.  Perhaps

  #define SPECPDL_INDEX()	((unsigned long) (specpdl_ptr - specpdl))

Although I doubt that someone will have more than 2GB or
unwind_protect'ed methods, but it's probably cleaner that way, because
it doesn't require one reason about the possible magnitude of the
values.

Re: EMACS_INT cleanup

[Eli Zaretskii]

> Date: Thu, 23 Sep 2010 22:46:09 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: emacs-devel@gnu.org
> 
> Better make it a type that is 64-bit wide on a 64-bit host.  Perhaps
> 
>   #define SPECPDL_INDEX()	((unsigned long) (specpdl_ptr - specpdl))

On second thought, maybe you are right, and casting to int is better.
Perhaps just leave a comment there explaining why.

Re: EMACS_INT cleanup

[Lars Magne Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> If ptrdiff_t works, it is a better solution, because SPECPDL_INDEX is
> defined like this:
>
>     extern struct specbinding *specpdl;
>     extern struct specbinding *specpdl_ptr;
>     ...
>     #define SPECPDL_INDEX()	(specpdl_ptr - specpdl)
>
> So it returns a difference between two pointers, which is exactly what
> ptrdiff_t is for (as its name hints).
>
> Casting it to an int will DTWT on a 64-bit host.

Makes sense.

I'll try ptrdiff_t and see what happens.

Calls like 

  int count = SPECPDL_INDEX ();

should probably be changed then, too.  
  
-- 
(domestic pets only, the antidote for overdose, milk.)
  larsi@gnus.org * Lars Magne Ingebrigtsen

Re: EMACS_INT cleanup

[Lars Magne Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> On second thought, maybe you are right, and casting to int is better.
> Perhaps just leave a comment there explaining why.

Ok.  :-)  I'll try recompiling with that, and see whether anything
exciting happens...

-- 
(domestic pets only, the antidote for overdose, milk.)
  larsi@gnus.org * Lars Magne Ingebrigtsen

Re: EMACS_INT cleanup

[Lars Magne Ingebrigtsen]

I've been -Wconversion-checking a bit more here and there.

Do we want to allow strings to be more than 2GB big?  There are as many
int/EMACS_INT problems in that area as in the buffer area, but it feels
kinda...  er...  not very useful to fix up format3() to allow format
statements to be EMACS_INT clean.

Just to take an example.

I mean, it may be nice and orthogonal and stuff, but...  I don't know.
And does using more longs on 64-bit machines than previously slow Emacs
down any?

-- 
(domestic pets only, the antidote for overdose, milk.)
  larsi@gnus.org * Lars Magne Ingebrigtsen

Re: EMACS_INT cleanup

[Juanma Barranquero]

On Fri, Sep 24, 2010 at 00:07, Lars Magne Ingebrigtsen <larsi@gnus.org> wrote:

> Do we want to allow strings to be more than 2GB big?

If we ever support buffers > 2GB, (buffer-string) > 2GB.

Not likely to be used, but you never know.

    Juanma

Re: EMACS_INT cleanup

[Stefan Monnier]

> Better make it a type that is 64-bit wide on a 64-bit host.  Perhaps
>   #define SPECPDL_INDEX()	((unsigned long) (specpdl_ptr - specpdl))

What's wrong with ptrdiff_t?


        Stefan

Re: EMACS_INT cleanup

[Lars Magne Ingebrigtsen]

Juanma Barranquero <lekktu@gmail.com> writes:

> If we ever support buffers > 2GB, (buffer-string) > 2GB.

Yes.  But would anyone ever say

(format (buffer-string))

or

(message "%s" (buffer-string))

with a 2GB buffer?

Hm.  Well, actually, I guess it might happen inadvertently.

So.  Fix up or not fix up?  

-- 
(domestic pets only, the antidote for overdose, milk.)
  larsi@gnus.org * Lars Magne Ingebrigtsen

Re: EMACS_INT cleanup

[Lars Magne Ingebrigtsen]

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

> What's wrong with ptrdiff_t?

In principle, that's the best thing.  However:

grep -nH -e SPECPDL_INDEX *.c
alloc.c:4849:  int count = SPECPDL_INDEX ();
alloc.c:4877:  int count = SPECPDL_INDEX ();
buffer.c:1405:    int count = SPECPDL_INDEX ();
buffer.c:1680:  count = SPECPDL_INDEX ();
buffer.c:3807:  int count = SPECPDL_INDEX ();
buffer.c:3916:  int count = SPECPDL_INDEX ();
bytecode.c:408:  int count = SPECPDL_INDEX ();
[165 lines removed]

Isn't the SPECPDL thing for building a backtrace strack?  The
specbinding structure is 16 bytes, and I'm assuming that it's organised
as a, er, stack.  So having this be more than 31 bits will require a
recursion that goes deeper than the moon has atoms.  (Well, ok.  That's
a slight exaggeration.  I admit it.)

Of course, I've probably totally misunderstood how is works after
spending several seconds reading the comment about it in lisp.h.

-- 
(domestic pets only, the antidote for overdose, milk.)
  larsi@gnus.org * Lars Magne Ingebrigtsen

Re: EMACS_INT cleanup

[Stefan Monnier]

> Isn't the SPECPDL thing for building a backtrace strack?  The
> specbinding structure is 16 bytes, and I'm assuming that it's organised
> as a, er, stack.  So having this be more than 31 bits will require a
> recursion that goes deeper than the moon has atoms.  (Well, ok.  That's
> a slight exaggeration.  I admit it.)

Yes, but using ptrdiff_t will have the advantage that it's just right.
Using int requires a cast and an argument for why int is sufficiently
large in practice.  Casts are bad, and human-arguments are only OK when
we can't argue directly with the compiler.


        Stefan

Re: EMACS_INT cleanup

[Stefan Monnier]

> I mean, it may be nice and orthogonal and stuff, but...  I don't know.
> And does using more longs on 64-bit machines than previously slow Emacs
> down any?

AFAIK using 64bit local variables on 64bit systems should have as only
cost the added stack use when spilling those vars on the stack.


        Stefan

Re: EMACS_INT cleanup

[Lars Magne Ingebrigtsen]

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

> Yes, but using ptrdiff_t will have the advantage that it's just right.
> Using int requires a cast and an argument for why int is sufficiently
> large in practice.  Casts are bad, and human-arguments are only OK when
> we can't argue directly with the compiler.

Yes, I agree.  It would be cleaner to fix the users to all be
ptrdiff_t.

-- 
(domestic pets only, the antidote for overdose, milk.)
  larsi@gnus.org * Lars Magne Ingebrigtsen

Re: EMACS_INT cleanup

[Eli Zaretskii]

> From: Lars Magne Ingebrigtsen <larsi@gnus.org>
> Date: Fri, 24 Sep 2010 00:07:32 +0200
> 
> Do we want to allow strings to be more than 2GB big?

IMO, definitely.  Quite a few Emacs features make a string from buffer
text and then put the string back.  So if we don't support large
strings, we have bugs waiting to happen.

> There are as many int/EMACS_INT problems in that area as in the
> buffer area

Yes, fixed a few yesterday.

> but it feels kinda...  er...  not very useful to fix up format3() to
> allow format statements to be EMACS_INT clean.

If you mean the %d vs %ld etc., then I don't think it's a frequent
problem in Emacs sources.

> I mean, it may be nice and orthogonal and stuff, but...  I don't know.
> And does using more longs on 64-bit machines than previously slow Emacs
> down any?

It shouldn't, the registers are 64-bit.

Re: EMACS_INT cleanup

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Lars Magne Ingebrigtsen <larsi@gnus.org>,  emacs-devel@gnu.org
> Date: Fri, 24 Sep 2010 00:30:20 +0200
> 
> > Better make it a type that is 64-bit wide on a 64-bit host.  Perhaps
> >   #define SPECPDL_INDEX()	((unsigned long) (specpdl_ptr - specpdl))
> 
> What's wrong with ptrdiff_t?

Nothing (it was my original suggestion), except that it's more work
for little or no gain.  We normally put the result of SPECPDL_INDEX
into an int, so using ptrdiff_t would mean change all those variables
into ptrdiff_t as well -- and for no good reason, because we can
hardly have so many unwind_protect'ed levels.

Re: EMACS_INT cleanup

[Eli Zaretskii]

> From: Lars Magne Ingebrigtsen <larsi@gnus.org>
> Date: Fri, 24 Sep 2010 00:53:40 +0200
> 
> Isn't the SPECPDL thing for building a backtrace strack?

No, it's for unwind_protect and specbind/unbind_to.

Re: EMACS_INT cleanup

[Andreas Schwab]

Lars Magne Ingebrigtsen <larsi@gnus.org> writes:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>> Yes, but using ptrdiff_t will have the advantage that it's just right.
>> Using int requires a cast and an argument for why int is sufficiently
>> large in practice.  Casts are bad, and human-arguments are only OK when
>> we can't argue directly with the compiler.
>
> Yes, I agree.  It would be cleaner to fix the users to all be
> ptrdiff_t.

Or a dedicated typedef.  The ptrdiff_t is an implementation detail of
the SPECPDL_SIZE macro.  Also, specpdl_size should be adjusted.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: EMACS_INT cleanup

[Miles Bader]

Eli Zaretskii <eliz@gnu.org> writes:
>> I mean, it may be nice and orthogonal and stuff, but...  I don't know.
>> And does using more longs on 64-bit machines than previously slow Emacs
>> down any?
>
> It shouldn't, the registers are 64-bit.

Means more cache usage and memory traffic tho.

Whether it's enough to matter, I dunno.

-Miles

-- 
You can hack anything you want, with TECO and DDT.

Re: EMACS_INT cleanup

[Eli Zaretskii]

> From: Miles Bader <miles@gnu.org>
> Cc: Lars Magne Ingebrigtsen <larsi@gnus.org>, emacs-devel@gnu.org
> Date: Fri, 24 Sep 2010 18:43:12 +0900
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> >> I mean, it may be nice and orthogonal and stuff, but...  I don't know.
> >> And does using more longs on 64-bit machines than previously slow Emacs
> >> down any?
> >
> > It shouldn't, the registers are 64-bit.
> 
> Means more cache usage and memory traffic tho.

Maybe, maybe not.  It depends on the micro-architecture of the CPU and
the cache.

As for memory, I don't think it matters for isolated variables (as
opposed to arrays).  They will be usually in a register when they are
used.

Re: EMACS_INT cleanup

[Eli Zaretskii]

> From: Lars Magne Ingebrigtsen <larsi@gnus.org>
> Date: Fri, 24 Sep 2010 00:07:32 +0200
> 
> I've been -Wconversion-checking a bit more here and there.

Can you commit your changes, or at least tell on which files you are
working?  I don't want us to work on the same files i parallel.

TIA

Re: EMACS_INT cleanup

[Lars Magne Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> I've been -Wconversion-checking a bit more here and there.
>
> Can you commit your changes, or at least tell on which files you are
> working?  I don't want us to work on the same files i parallel.

I'm not working on anything at the moment -- I've been sleeping.  :-)

I have no outstanding EMACS_INT fixes, but I went through a couple files
just getting a feel for what kind of thing is giving the most long->int
conversion warnings, and it's definitely a lot in the "hmmm" area in
addition to the simpler int i = PV; area.

Like:

void
message3 (Lisp_Object m, int nbytes, int multibyte)

So we can only do a message that's 2GB big?  Well, OK, but is that a
problem? 

-- 
(domestic pets only, the antidote for overdose, milk.)
  larsi@gnus.org * Lars Magne Ingebrigtsen

Re: EMACS_INT cleanup

[Eli Zaretskii]

> From: Lars Magne Ingebrigtsen <larsi@gnus.org>
> Date: Fri, 24 Sep 2010 13:02:43 +0200
> 
> void
> message3 (Lisp_Object m, int nbytes, int multibyte)
> 
> So we can only do a message that's 2GB big?  Well, OK, but is that a
> problem? 

Why bother thinking?  Why not just replace int with EMACS_INT?

Re: EMACS_INT cleanup

[Lars Magne Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> void
>> message3 (Lisp_Object m, int nbytes, int multibyte)
>> 
>> So we can only do a message that's 2GB big?  Well, OK, but is that a
>> problem? 
>
> Why bother thinking?  Why not just replace int with EMACS_INT?

I'm just querying the list about what they think the general policy for
this should be.  I (personally) think all this should be fixed, so that
the gcc warnings would actually be useful, but since -Wall is in the
state it's in, I was assuming that there might be a reason for that
state.

-- 
(domestic pets only, the antidote for overdose, milk.)
  larsi@gnus.org * Lars Magne Ingebrigtsen

Re: EMACS_INT cleanup

[Eli Zaretskii]

> From: Lars Magne Ingebrigtsen <larsi@gnus.org>
> Date: Fri, 24 Sep 2010 15:00:07 +0200
> 
> since -Wall is in the state it's in, I was assuming that there might
> be a reason for that state.

Lazyness^H^H^H^H^H^Hck of time?

Re: EMACS_INT cleanup

[Lars Magne Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> since -Wall is in the state it's in, I was assuming that there might
>> be a reason for that state.
>
> Lazyness^H^H^H^H^H^Hck of time?

Right.  :-)

I'll continue the -Wconversion trolling in the Emacs sources, and fix
all the long->int conversions I find, no matter how unlikely it is to
appear in real life, then.

But probably not tonight.

If somebody else wants to join in, feel free.  They methodology I'm
using is the following:

1) First I say "make" as normal in the src directly.

2) Then I alter the warnings switch to add -Wconversion:

C_WARNINGS_SWITCH = -Wimplicit-function-declaration -Wold-style-definition -Wdeclaration-after-statement -Wno-pointer-sign -Wconversion

3) Then I pick a file -- for instance, fns.c.  Then I force-save it to
update the time stamp.

4) Then I hit F7, which I have bound to:

(global-set-key
 [f7] (lambda () (interactive)
	(save-some-buffers t)
        (ignore-errors
	  (mapcar (lambda (proc) (kill-process proc))
		  compilation-in-progress))
	(setq compilation-in-progress nil)
	(compile "make -k 2>&1 | egrep \"(conversion.*'int'.*(Lisp_Obj|long)|error)\"")))

So then it's only a case of next-erroring to each error and fix stuff.

Anyone can do it!  :-)

(If they have gcc 4.4.)
        
-- 
(domestic pets only, the antidote for overdose, milk.)
  larsi@gnus.org * Lars Magne Ingebrigtsen

Re: EMACS_INT cleanup

[Eli Zaretskii]

> From: Lars Magne Ingebrigtsen <larsi@gnus.org>
> Date: Fri, 24 Sep 2010 15:23:16 +0200
> 
> 3) Then I pick a file -- for instance, fns.c.  Then I force-save it to
> update the time stamp.

This step is not needed.  You can instead use the -W switch to Make:

  cd src && make FOO.o -W FOO.c

Re: EMACS_INT cleanup

[Lars Magne Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> This step is not needed.  You can instead use the -W switch to Make:
>
>   cd src && make FOO.o -W FOO.c

Nice.  The f7 macro could be extended to do that automatically.

Oh, and I also have

(global-set-key [f9] 'next-error)

`C-x `' is just way too much work.

And of course etags is useful for jumping around and stuff, but I
everybody already does that, I guess...

-- 
(domestic pets only, the antidote for overdose, milk.)
  larsi@gnus.org * Lars Magne Ingebrigtsen

Re: EMACS_INT cleanup

[Lars Magne Ingebrigtsen]

Lars Magne Ingebrigtsen <larsi@gnus.org> writes:

> Nice.  The f7 macro could be extended to do that automatically.

So now the version of the macro look like:

(global-set-key
 [f7] (lambda () (interactive)
	(save-some-buffers t)
        (ignore-errors
	  (mapcar (lambda (proc) (kill-process proc))
		  compilation-in-progress))
	(setq compilation-in-progress nil)
	(compile
	 (format "make CFLAGS='-g -O2  -Wconversion' -k %s -W %s 2>&1 | egrep \"(conversion.*'int'.*(Lisp_Obj|long)|error)\""
		 (file-name-nondirectory
		  (replace-regexp-in-string ".c$" ".o" (buffer-file-name)))
		 (file-name-nondirectory (buffer-file-name))))))

And you don't even have to alter the Makefile to add the -Wconversion
any more.
                 
-- 
(domestic pets only, the antidote for overdose, milk.)
  larsi@gnus.org * Lars Magne Ingebrigtsen

Re: EMACS_INT cleanup

[David Kastrup]

Lars Magne Ingebrigtsen <larsi@gnus.org> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>> This step is not needed.  You can instead use the -W switch to Make:
>>
>>   cd src && make FOO.o -W FOO.c
>
> Nice.  The f7 macro could be extended to do that automatically.
>
> Oh, and I also have
>
> (global-set-key [f9] 'next-error)
>
> `C-x `' is just way too much work.

M-g M-n should work equally well.

-- 
David Kastrup