On Friday 25 July 2008 01:12:55 Christoph Otto via RT wrote: > According to make cover, the case where the return was -1 was never hit. > A simple test case caused a segfault, which definitely isn't the right > thing. The attached patch fixes and refactors intlist_get() to make the > coverage more obvious in the output of the coverage tests. It also adds > a test case to exercise the previously neglected path. > > I'd like comments on whether intlist_get does the right thing with this > patch and if it's exercised correctly by the test.
IntList PMCs can resize, so it looks correct to me. > Index: src/intlist.c > =================================================================== > --- src/intlist.c (revision 29733) > +++ src/intlist.c (working copy) > @@ -306,10 +306,13 @@ > INTVAL > intlist_get(PARROT_INTERP, ARGMOD(IntList *list), INTVAL idx) > { > - /* XXX list_get can return NULL RT #48367 */ > void * const ret = list_get(interp, (List *)list, idx, > enum_type_INTVAL); - const INTVAL retval = ret == (void *)-1 ? 0 : > *(INTVAL *)ret; - return retval; > + if (ret == NULL) { > + return (INTVAL)0; > + } > + else { > + return *(INTVAL *)ret; > + } > } I'd write that as: if (ret) return *(INTVAL *)ret; return (INTVAL)0; The pointer casting dereferencing bothers me a little, but if compilers don't warn about it.... -- c