Version: 1.2.0_devel (using KDE KDE 3.4.2) Installed from: Compiled From Sources Compiler: gcc (GCC) 3.4.5 OS: Linux There's an unsigned/signed ambiguity in getdata's implementation of the char ('c') type and I'm not sure which one is desired. Comments in getdata.h and the ConvertType() function consider 'c' to refer to unsigned chars. Comments preceeding GetData() and the Add/Scale/Multiply/Linterp/&c.-data() functions, as well as FillFileFrame(), indicate 'c' to be signed. Which one should it be?
'c' Should be unsigned char.
Should be fixed for 1.2.1 release
Created attachment 14395 [details] unsigned char patch Here's a patch.
I wonder how portable this is. char is unsigned on some platforms, signed on others.
George, could you give more details on your concerns. Isn't "unsigned char" unsigned on all platforms? I can see we might have a problem with the existing code, but the proposed patch seems to address those issues.
On Monday 13 March 2006 14:53, Andrew Walker wrote: > ------- George, could you give more details on your concerns. Isn't > "unsigned char" unsigned on all platforms? I can see we might have a > problem with the existing code, but the proposed patch seems to address > those issues. The problems are: 1) Is it using unsigned char in every possible place? - needs investigation 2) Switching what's used at this point could break existing files. Then again, they might already be broken.
1) I would think this be easily confirmed as the patch is attached 2) The latter I would imagine
In all data getdata files I am aware of, chars are only used as bitfields... so unsigned char is fine. Don's patch is fine and should be committed.
SVN commit 518936 by arwalker: BUG:119531 Apply Don's patch M +12 -11 getdata.c --- trunk/extragear/graphics/kst/src/datasources/dirfile/getdata.c #518935:518936 @@ -526,7 +526,7 @@ switch(rtype) { case 'c': for (i=0; i<n; i++) { - ((char*)dataout)[i] = (char)i+s0; + ((unsigned char*)dataout)[i] = (unsigned char)i+s0; } break; case 'i': /* for compatibility with creaddata. (deprecated) */ @@ -1054,7 +1054,7 @@ /* */ /***************************************************************************/ static void ScaleData(void *data, char type, int npts, double m, double b) { - char *data_c; + unsigned char *data_c; short *data_s; unsigned short *data_u; unsigned *data_U; @@ -1068,9 +1068,9 @@ case 'n': break; case 'c': - data_c = (char *)data; + data_c = (unsigned char *)data; for (i=0; i<npts; i++) { - data_c[i] =(char)((double)data_c[i] * m + b); + data_c[i] =(unsigned char)((double)data_c[i] * m + b); } break; case 's': @@ -1129,7 +1129,7 @@ break; case 'c': for (i=0; i<n; i++) { - ((char*)A)[i] += ((char*)B)[i * spfB / spfA]; + ((unsigned char*)A)[i] += ((unsigned char*)B)[i * spfB / spfA]; } break; case 'S': case 'i': @@ -1183,7 +1183,7 @@ break; case 'c': for (i=0; i<n; i++) { - ((char*)A)[i] *= ((char*)B)[i * spfB / spfA]; + ((unsigned char*)A)[i] *= ((unsigned char*)B)[i * spfB / spfA]; } break; case 'S': case 'i': @@ -1556,10 +1556,11 @@ return; break; case 'c': - x = ((char *)data)[i]; + x = ((unsigned char *)data)[i]; idx = GetIndex(x, lx, idx, n_ln); - ((char *)data)[i] = (char)(ly[idx] + (ly[idx+1]-ly[idx])/ - (lx[idx+1]-lx[idx]) * (x-lx[idx])); + ((unsigned char *)data)[i] = + (unsigned char)(ly[idx] + (ly[idx + 1] - ly[idx]) / + (lx[idx + 1] - lx[idx]) * (x - lx[idx])); break; case 's': x = ((short *)data)[i]; @@ -1727,8 +1728,8 @@ /* num_frames, num_samps: the number of samples read is */ /* num_samps + samples_per_frame*num_frames */ /* return_type: data type of *data_out. 's': 16 bit signed */ -/* 'u' 16bit unsiged. 'S' 32bit signed 'U' 32bit unsigned */ -/* 'c' 8 bit signed */ +/* 'u' 16bit unsigned 'S' 32bit signed 'U' 32bit unsigned */ +/* 'c' 8 bit unsigned */ /* void *data_out: array to put the data */ /* *error_code: error code is returned here. If error_code==null, */ /* GetData prints the error message and exits */