Re: Windows 64 port

[Paul Eggert <eggert@cs.ucla.edu>]

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