Paul Eggert | 1 Mar 04:23 2012

Re: Windows 64 port

On 02/28/2012 01:32 PM, Fabrice Popineau wrote:

> If I define tzname as _tzname is ms-w32.h, I get an error when
> including MS time.h on some function returning an array.

It should be possible to work around this problem without having to
modify lib/strftime.c.  For example, src/s/ms-w32.h can do something like this:

   /* Include <time.h> before defining tzname, to avoid DLL clash.  */
   #include <time.h>
   #define tzname _tzname

This should avoid the diagnostic, since tzname isn't defined until
after <time.h> is included.  It's better to keep Microsoft-specific
stuff in Microsoft-specific source files when possible, as seems to
be the case here.

>     >  #ifdef WINDOWSNT
>     >  extern Lisp_Object w32_get_internal_run_time (void);
>     > +
>     > +extern struct tm *localtime (const time_t *t);
>     >  #endif
> 
>     Why is this needed?  It seems also unrelated to 64-bit Windows.
> 
> To remove a warning on localtime not define.

Surely this should be done via "#include <time.h>", not by repeating
some of the internals of time.h.

(Continue reading)

Paul Eggert | 1 Mar 04:34 2012

Re: Windows 64 port

On 02/29/2012 01:24 PM, Eli Zaretskii wrote:
> I see many changes that qualify existing
> declarations with `const', which cannot possibly be wrong.

It's not an issue of whether the patch is *wrong*.
It's whether the patch is *needed*.  I can think of
hundreds of places where I could put 'const' into the
Emacs code, without breaking Emacs.  But these changes
are irrelevant to the Windows 64 port, and we should
consider these changes independently of the Windows 64
changes.

It's a significant alteration to the Emacs internal
programming style to use "const" when possible
(instead of when it's required), and any such
change should be considered on its own merits.
I happen to prefer "const" myself, but I realize that
many folks consider it to be unnecessary verbiage,
and we shouldn't introduce "const" under the argument
that it's needed for Windows 64 when in fact it's
not needed for that.

> I also see
> replacements of `unsigned long' with a `size_t', which cannot be
> wrong, either.

Same thing.  Some of those replacements are needed for Windows
64, but many (most?) are not.  I addressed this point in more
detail in my previous message that I sent out a few minutes ago.

(Continue reading)

Eli Zaretskii | 1 Mar 05:03 2012
Picon

Re: Windows 64 port

> Date: Wed, 29 Feb 2012 19:34:37 -0800
> From: Paul Eggert <eggert <at> cs.ucla.edu>
> Cc: fabrice.popineau <at> supelec.fr, emacs-devel <at> gnu.org,
> 	ajmr <at> ilovetortilladepatatas.com
> 
> On 02/29/2012 01:24 PM, Eli Zaretskii wrote:
> > I see many changes that qualify existing
> > declarations with `const', which cannot possibly be wrong.
> 
> It's not an issue of whether the patch is *wrong*.
> It's whether the patch is *needed*.

That's not what you originally claimed.  You said many changes are to
pacify the Microsoft compiler's bogus warnings.  There's nothing bogus
about using the right data type or declare a variable or an argument
`const' when it is.

I agree that these changes should go in a separate commit.  But then I
asked several times to do the same for texinfo.tex when you sync with
gnulib, and you declined.  So I guess Fabrice is not the only one who
does that...

Paul Eggert | 1 Mar 07:28 2012

Re: Windows 64 port

On 02/29/2012 08:03 PM, Eli Zaretskii wrote:
> There's nothing bogus about using the right data type

But the code in question is already using a correct data type.
That is, for the diffs marked "Not needed for Windows 64" in
<http://lists.gnu.org/archive/html/emacs-devel/2012-02/msg00762.html>,
Emacs is already using data types that work,
both on mainstream platforms and on Windows 64.

>  or declare a variable or an argument `const' when it is.

Suppose someone proposed lots of patches like this:

-  size_t len = strlen (string);                                                
+  size_t const len = strlen (string);

on the grounds that the XYZ Corp. compiler warns whenever
code fails to use 'const' at every opportunity.
That would be bogus -- that's not the Emacs
programming style, and we shouldn't slavishly alter
mainstream code merely to pacify a third-party compiler
that prefers a different style.

> I asked several times to do the same for texinfo.tex when you sync with
> gnulib, and you declined.

Those patches were automatically merged from an already-debugged
and well-modularized upstream, and it's trivial to see which parts
affect texinfo.tex.

(Continue reading)

Fabrice Popineau | 1 Mar 08:04 2012
Picon
Picon

Re: Windows 64 port

Suppose someone proposed lots of patches like this:

-  size_t len = strlen (string);
+  size_t const len = strlen (string);

on the grounds that the XYZ Corp. compiler warns whenever
code fails to use 'const' at every opportunity.
That would be bogus -- that's not the Emacs
programming style, and we shouldn't slavishly alter
mainstream code merely to pacify a third-party compiler
that prefers a different style.


The const fixes I proposed are easily isolated: they affect only the src/regex.c file.
You are free to submit it or to forget about it.
In this file, const is used in a few places only, resulting in dozens of warnings. Either
remove the use of const or use it everywhere it is needed. 
There is always the option to #define const to nothing, but I won't favor it.

Fabrice
Fabrice Popineau | 1 Mar 08:24 2012
Picon
Picon

Re: Windows 64 port



2012/3/1 Paul Eggert <eggert <at> cs.ucla.edu>
On 02/28/2012 01:32 PM, Fabrice Popineau wrote:

> If I define tzname as _tzname is ms-w32.h, I get an error when
> including MS time.h on some function returning an array.

It should be possible to work around this problem without having to
modify lib/strftime.c.  For example, src/s/ms-w32.h can do something like this:

  /* Include <time.h> before defining tzname, to avoid DLL clash.  */
  #include <time.h>
  #define tzname _tzname

This should avoid the diagnostic, since tzname isn't defined until
after <time.h> is included.  It's better to keep Microsoft-specific
stuff in Microsoft-specific source files when possible, as seems to
be the case here.


The problem is trickier than this and probably need a careful rewrite of ms-w32.h.
In this file, names like tzname or fopen get rewired. Some of them need to
be rewired to sys_fopen or _fopen (example) depending on whether it is for emacs or not.
So a more robust way of doing things would be to ensure that all system headers are read once
for all, then doing the rewiring for emacs and for (not emacs). That should avoid the problem
of tzname for example. In the mean time, this is the smallest change I could come with.
What you propose didn't work (resulting in either : some function returns an array, that is tzname, 
or _tzname being undefined at link time).
 
>     >  #ifdef WINDOWSNT
>     >  extern Lisp_Object w32_get_internal_run_time (void);
>     > +
>     > +extern struct tm *localtime (const time_t *t);
>     >  #endif
>
>     Why is this needed?  It seems also unrelated to 64-bit Windows.

Sure but the definition needs to be put somewhere because an int is not the same
size as a pointer in windows 64.

 
> - const problems in regex.c (large patch, but harmless)

None of the regex.c changes are needed for Windows 64.  The code works

I anwsered in a separate message. I never claimed it is needed by Windows 64 either.
(My only claim is that the whole thing I sent is what I needed to get a clean compile.)

> - turn off compiler warnings of 3 kinds (harmless, but frequent in emacs code)

Not needed for Windows 64, since these warnings are harmless.

Sure, but unless the source code is fixed to avois the warnings, it gets me a cleaner compilation listing.

 
Here is a more-detailed review of the latest patch you sent.

> -      int i = 0, aligned = (intptr_t) ABLOCKS_BUSY (abase);
> +      int i = 0, aligned = (ABLOCKS_BUSY (abase) != NULL);

Not needed for Windows 64.  The change slows the code down a bit, and

Prove it. I bet your compiler is much more clever than you think. twenty years ago, I would 
have agree, not today.
 
it might not work on unusual hosts that have filler bits in their
pointers.

The it is because the compiler is buggy, because this is a perfectly legal and much more
right than the double cascading cast (one explicit,  one implicit).

> -allocate_pseudovector (int memlen, int lisplen, EMACS_INT tag)
> +allocate_pseudovector (size_t memlen, size_t lisplen, EMACS_INT tag)

Not needed for Windows 64, since the arguments are all in int range.

Who knows that ? Certainly not the compiler.
 
 
> -         int size = min (2000, max (20, (nextb->text->z_byte / 10)));
> +         size_t size = min (2000, max (20, (nextb->text->z_byte / 10)));

Not needed for Windows 64, since 'size' is in int range.
 
Same remark.
 
> -             make_gap (-(nextb->text->gap_size - size));
> +             make_gap ((size - nextb->text->gap_size));

Not needed for Windows 64, since the expressions are equivalent.

Depends on signed/unsigned. 

 
> -  unsigned long int adj;
> +  intptr_t adj;

Not needed for Windows 64; the value is always in unsigned long range.


I thank you for all those intptr_t -> uintptr_t catches.
 
> +#ifdef min
> +#undef min
> +#endif


Just because it is redefined when it is a commonly defined macro in system headers.
 
> -    unsigned long int magic; /* Magic number to check header integrity.  */
> +    uint64_t magic;  /* Magic number to check header integrity.  */

Not needed for Windows 64, since the magic number fits in 32 bits.

If it is a 32bits integer, then better make it this way everywhere.
 

> -extern struct Lisp_Vector *allocate_pseudovector (int memlen, int lisplen, EMACS_INT tag);
> +extern struct Lisp_Vector *allocate_pseudovector (size_t memlen, size_t lisplen, EMACS_INT tag);

Not needed for Windows 64, since the values fit in 'int'.


Again, the compiler does not know it.
> +#define SIZE ESIZE

I don't see the reason for this #define; could you please explain?
If it is needed, let's just dump SIZE entirely, and use size_t instead.


Not needed, that was a redifinition of a system header macro.

 
> -long
> +intptr_t
>  r_alloc_size_in_use (void)

ptrdiff_t, not intptr_t, since it's arrived at by subtracting pointers.

OK.
 

>  #ifdef REL_ALLOC
> -  extern POINTER (*real_morecore) (long);
> +  extern POINTER (*real_morecore) (__malloc_ptrdiff_t);
>  #endif
> -  extern POINTER (*__morecore) (long);
> +  extern POINTER (*__morecore) (__malloc_ptrdiff_t);

I don't see how this change could work, since __malloc_ptrdiff_t is
not visible in vm-limit.c.  I suggest just using ptrdiff_t.


It should be else I won't have been able to compile it. Anyway no problem with that.
I'm not even sure why there is a __malloc_ptrdiff_t at all.

Fabrice
Miles Bader | 1 Mar 09:46 2012
Picon

Re: Possessive apostrophe with singular nouns ending in "s"

Glenn Morris <rgm <at> gnu.org> writes:
> A critical question. What is the standard form in GNU manuals for using
> the possessive apostrophe with singular nouns ending in "s"?
>   This is one of Emacs's most important features.
>   This is one of Emacs' most important features.

What I've always heard (and which seems eminently practical) is that
one should just follow the actual pronunciation, which is pretty easy
for a native-speaker to check.  Most things get an explicit "'s" (so
it makes sense for that to be the default if it's unclear), but some
words just sound downright weird that way, so it should be omitted for
them.

[Emacs sounds OK either way to me, but I'd say "Emacs's" should get
the nod...]

-miles

--

-- 
"I distrust a research person who is always obviously busy on a task."
   --Robert Frosch, VP, GM Research

Paul Eggert | 1 Mar 09:58 2012

Re: Windows 64 port

On 02/29/2012 11:24 PM, Fabrice Popineau wrote:

>     For example, src/s/ms-w32.h can do something like this:
> 
>       /* Include <time.h> before defining tzname, to avoid DLL clash.  */
>       #include <time.h>
>       #define tzname _tzname
> 
>     This should avoid the diagnostic, since tzname isn't defined until
>     after <time.h> is included.

> What you propose didn't work (resulting in either : some function
> returns an array, that is tzname, or _tzname being undefined at link
> time).

Sorry, I don't follow.  What exactly happens when you try the above
suggestion, and why?  And what does this have to do with functions
returning arrays?

Perhaps you can explain, by showing the preprocessor output of
the failed attempt.

> the definition needs to be put somewhere because an int is not the same
> size as a pointer in windows 64.

OK, thanks, it appears you fixed that in a different way in the last
patch, so the point is moot.

> unless the source code is fixed to avois the warnings,
> it gets me a cleaner compilation listing.

Surely there's some way to tell the compiler to not issue the warnings.
Or you can simply ignore these bogus messages.

>     Here is a more-detailed review of the latest patch you sent.
> 
>     > -      int i = 0, aligned = (intptr_t) ABLOCKS_BUSY (abase);
>     > +      int i = 0, aligned = (ABLOCKS_BUSY (abase) != NULL);
> 
>     Not needed for Windows 64.  The change slows the code down a bit, and
> 
> 
> Prove it.

I compiled it both ways (x86-64 GCC 4.6.2), and the "!= NULL" version
has an extra "cmpl" instruction, so it is indeed a bit fatter.

>     it might not work on unusual hosts that have filler bits in their
>     pointers.
> 
> The it is because the compiler is buggy, because this is a perfectly legal and much more
> right than the double cascading cast (one explicit,  one implicit).

Sorry, it sounds like my suggestion wasn't clear.  On some
architectures, pointers have unused bits, there are multiple
representations of NULL, and the test for whether a pointer is NULL is
not the same as testing whether its bit-pattern is all zeros.
Compilers on such systems are not buggy: they are simply implementing
the machine's architecture.

Admittedly this point is probably theoretical.  As far as I know among
Emacs targets only the s390 architecture has unused bits in its
pointers, and I don't know whether its comparisons to NULL ignore
those bits.  Still, the point remains that one must be careful when
making changes like this, and there's no need for this particular
change for the Windows 64 port.

>     Not needed for Windows 64, since the arguments are all in int range.
> 
> Who knows that ? Certainly not the compiler.

*You* know it, because I just told you.  (:-)  I've read the code.

Similarly for your other remarks about the compiler not knowing
things.  In all these cases, the compiler may not be smart enough to
figure things out, but the values are indeed in range and there's no
need to alter the types.

>     > -             make_gap (-(nextb->text->gap_size - size));
>     > +             make_gap ((size - nextb->text->gap_size));
> 
>     Not needed for Windows 64, since the expressions are equivalent.
> 
> Depends on signed/unsigned. 

On Windows 64 the two expressions are equivalent, so there's no need
for this patch in the Windows 64 port.

>     > -  unsigned long int adj;
>     > +  intptr_t adj;
> 
>     Not needed for Windows 64; the value is always in unsigned long range.
> 
> I thank you for all those intptr_t -> uintptr_t catches.

You're welcome, but this is not an example of that.  The declaration
does not need to be changed, because the value of 'adj' is always
in the range 0..ULONG_MAX, on Windows 64 and on mainline hosts.

>     > +#ifdef min
>     > +#undef min
>     > +#endif
> 
> Just because it is redefined when it is a commonly defined macro in
> system headers.

Which system header defined it, and what exactly was it defined to?
If a system header defines 'min' it may not be wise to change it.

>     > -    unsigned long int magic; /* Magic number to check header integrity.  */
>     > +    uint64_t magic;  /* Magic number to check header integrity.  */
> 
>     Not needed for Windows 64, since the magic number fits in 32 bits.
> 
> If it is a 32bits integer, then better make it this way everywhere.

Perhaps -- but then why use uint64_t for a 32-bit quantity?
And anyway, the patch is not needed for Windows 64, regardless of what
type (if any) is substituted for unsigned long.

> I'm not even sure why there is a __malloc_ptrdiff_t at all.

It's a relic of the older pre-C89 environment.  These days we
should just remove __malloc_ptrdiff_t and use ptrdiff_t.
(But as part of a separate patch; it's not needed for Windows 64.)

Thien-Thi Nguyen | 1 Mar 10:25 2012

info handling dir.gz

This patch is required for GNU Emacs to read /usr/share/info/dir.gz
(et al).  I hope it can make it into the next release.

[Insert excuse for not having installed Bazarre, here
 (flames => /dev/null).]

______________________________________________________________
Attachment (info.diff): text/x-diff, 737 bytes
Tom | 1 Mar 11:21 2012
Picon

Humane interface for changing values

Check out this video from 4:00 to see a handy way of playing with
values in the source code, changing colors and stuff:

http://vimeo.com/36579366

It can be good inspiration to implement an emacs package that provides
something like this.


Gmane