[GRASS-dev] Re: About the vector changes

[Markus Metz]

Glynn Clements wrote:
> Markus Metz wrote:
>
>   
>>> [...] An integer literal without a
>>> trailing suffix is an "int". Using a larger type to hold the value
>>> where necessary is a gcc extension. Other compilers may simply
>>> truncate the value to an int, even if off_t is 64 bits.
>>>
>>> The latest change:
>>>
>>> 	#define OFF_T_TEST 0x0102030405060708LL
>>>
>>> will fail to compile with a compiler which doesn't have long long int.
>>> Both the definition and use of OFF_T_TEST need to be conditionalised
>>> with "#ifdef HAVE_LONG_LONG_INT".
>>>       
>> OK. But on Linux 32 without LFS I get
>>
>> port_init.c:217: warning: overflow in implicit constant conversion
>>
>> at compile time because long long int is available but sizeof(off_t) = 
>> 4. Portability and its test is ok because OFF_T_TEST is not used in this 
>> case.
>>     
>
> Making the conversion explicit would eliminate the warning:
>
> 	u_o = (off_t) OFF_T_TEST;
>   
OK.
> [...]
>  A 64-bit system might have a 64-bit off_t
> but no "long long" type. We really need to have autoconf use define
> SIZEOF_LONG etc in config.h (AC_CHECK_SIZEOF macro), so that we can
> use e.g.:
>
> 	#if SIZEOF_LONG == 8
> 	#define LONG_LONG_TEST 0x0102030405060708L
> 	#elif defined(HAVE_LONG_LONG_INT)
> 	#define LONG_LONG_TEST 0x0102030405060708LL
> 	#endif
>   
You can take it from gdal.
>
>>> I still see:
>>>
>>> 719:	    s[top].sn.branch[j].child.id =
>>> 720:		(int)s[top].childpos[j];
>>>
>>> 754:				s[top].sn.branch[j].child.id =
>>> 755:				    (int)s[top].childpos[j];
>>>
>>> Also:
>>>
>>> 1011:			if (!shcb((int)s[top].sn.branch[i].child, cbarg)) {
>>>
>>> A narrowing cast always brings up the question of "what if the value
>>> won't fit in the destination type"?
>>>
>>> Am I missing something, or is childpos[j] *always* an "int"? 
>>>       
>> On level 0, it is always an int. It's the ID of the rectangle passed to
>> int RTreeInsertRect(struct Rect *r, int tid, struct RTree *t)
>> as tid. That applies to all three cases above.
>>
>> I am writing out the rectangle ID as off_t although it is int and have 
>> to read it as off_t then cast it to int again when loading the sidx file 
>> to memory for updating. Of course I could also write the ID as int and 
>> read it as int, then there would be no type casting, at the cost of one 
>> more if ... else ... .
>>
>>     
>>> If so,
>>> why is it stored in the file as an off_t?
>>>       
>> On higher levels (RTree internal nodes), it is the position in file of 
>> the child nodes of a node. For large sidx files, off_t is needed. The 
>> corresponding line in spindex_rw.c is
>>
>> 668 s[top].pos[s[top].branch_id - 1] = nextfreepos;
>>
>> where I set the node positions in file as off_t.
>>
>> This is related to the use of union Child: on level 0, int id is used, 
>> on higher levels, struct Node *ptr is used.
>>     
>
> So it's a case of simplifying the sidx file I/O?
>   
Yes, partly my laziness. Took me ages to get that particular post-order 
traversal right.
> That's reasonable enough. But, personally, I would:
>
> 1. Make the pos and childpos fields unions (int/off_t).
>   
That's now easy, similar to the union in rtree.
> 2. Add an explicit range check before casting the off_t read from the
> file to an int. 
That should only be necessary if there is reason to suspect that the 
sidx file is not read properly. Hmm, actually that would be a good check 
for exactly that.
> An assert() would suffice; the main purpose is to
> answer the "what if it doesn't fit in an int?" question for the
> benefit of anyone trying to read that code.
>   
Only works if off_t_size = 8 is both available and used, otherwise it 
will obviously always fit into an int.
The answer would be that the programmer wrote bad code with bugs because 
it should never happen that it doesn't fit into an int.

Markus M