[GRASS-dev] Re: About the vector changes

[Markus Metz]

Glynn:
> [...]
>
> However, as a result of the LFS changes, the code has some type
> mismatches on 32-bit systems with LFS enabled.
>
> Most of these are due to passing off_t's to G_debug() with %ld as the
> format (it needs to be %lld for a 64-bit off_t on a 32-bit platform;
> we really need a macro for this). There's also one G_fatal_error()
> call which has "%d".
>   
There was something about long long int, it's part of the C99 standard 
but not supported by all compilers if I remember correctly. That's the 
reason for HAVE_LONG_LONG_INT?
> These shouldn't actually cause any problems beyond reporting bogus
> offsets in debug/error messages. On little-endian platforms, the
> offset will be truncated to 32 bits (so it has no effect for offsets
> <2GiB). It's more of an issue on big-endian platforms, as you get the
> high word, so offsets will be divided by 4GiB (i.e.they'll normally be
> zero).
>
> In lib/vector/diglib/port_init.c,
>
> 	#define OFF_T_TEST 0x0102030405060708
>
> should have an LL suffix (it's too large for an "int"), and this
> should ideally be conditionalised 
Its usage is conditionalised: it's only used if sizeof(off_t) == 8, 
otherwise OFF_T_TEST is ignored and LONG_TEST is used instead. Applies 
to port_test.c as well. The current portability test is passed on 32bit 
with LFS.
> (although I don't know if we have a
> macro for "off_t is 64-bits"; _FILE_OFFSET_BITS may not be defined on
> 64-bit platforms, and you can't use sizeof in #if tests). I believe
> that the compiler is free to truncate the above to an int, regardless
> of what it's assigned to. If that happens, the vector code will fail
> entirely.
>
> Finally:
>
> spindex_rw.c:657: warning: cast from pointer to integer of different size
>
> spindex_rw.c:715: warning: cast to pointer from integer of different size
> spindex_rw.c:746: warning: cast to pointer from integer of different size
>
> The first one is less of an issue; a platform with 64-bit pointers
> will probably have a 64-bit off_t. The converse isn't true; on a
> 32-bit system with LFS, storing a 64-bit off_t in a 32-bit pointer
> isn't going to work.
>
> The lines in question are:
>
> 656:		if (s[top].sn->level == 0)	/* leaf node */
> 657:		    s[top].pos[j] = (off_t) s[top].sn->branch[j].child;
>
> 714:	    s[top].sn.branch[j].child =
> 715:		(struct Node *)s[top].childpos[j];
>
> 745:			    s[top].sn.branch[j].child =
> 746:				(struct Node *)s[top].childpos[j];
>
> ISTR that it should only cast childpos[j] to a pointer if that is what
> was stored there in the first place. 
In line 746, I was stuffing an off_t value that is smaller than INT_MAX 
into a pointer. There was an integer stored initially.
> But I would strongly suggest
> using a union rather than casting; 
You suggested that earlier already. First time I work with unions, done 
in trunk r38654. Could please have a look at lib/vector/rtree/index.h? 
The compiler warnings above about pointer <-> integer casts are gone, 
but I'm not sure if I used that union thing correctly.

Picking your brain, unrelated: can I assume that DBL_MAX is always 
available on all platforms? I would like to use it in lib/vector/rtree.

> we shouldn't expect people to
> simply ignore compiler warnings, and it's unreasonable to expect
> anyone to analyse the code to the extent that they can determine that
> the warnings are harmless.
>   
Agreed. Any volunteers out there who want to analyse the rtree code? 
That would actually be very welcome...

Markus M