[GRASS-dev] Re: About the vector changes

[Glynn Clements]

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;

> To avoid this warning I could use in port_init()
> 
> #ifdef HAVE_LONG_LONG_INT
> #ifdef HAVE_LARGEFILES
> u_o = OFF_T_TEST;
> #else
> u_o = LONG_TEST;
> #endif
> #else
> u_o = LONG_TEST;
> #endif

I suggest:

1. Remove OFF_T_TEST.

2. Add:

	#ifdef HAVE_LONG_LONG_INT
	#define LONG_LONG_TEST 0x0102030405060708LL
	#endif

3. Change the initialisation of u_o to e.g.:

	    if (nat_off_t == 8)
	#ifdef HAVE_LONG_LONG_INT
		u_o = (off_t) LONG_LONG_TEST;
	#else
		G_fatal_error("Internal error: can't construct an off_t literal");
	#endif
	    else
		u_o = (off_t) LONG_TEST;

Actually, that's not ideal. 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

> but I am not sure if HAVE_LARGEFILES is always set on 64bit systems also 
> without --enable-largefile.

It won't be set if --enable-largefile isn't used.

> > 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?

That's reasonable enough. But, personally, I would:

1. Make the pos and childpos fields unions (int/off_t).

2. Add an explicit range check before casting the off_t read from the
file to an int. 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.

-- 
Glynn Clements <glynn at gclements.plus.com>