On Mon, 28 Apr 2008 23:51:09 +0100, Tim Bunce <[EMAIL PROTECTED]> wrote: > On Mon, Apr 28, 2008 at 03:24:10PM +0100, Rudolf Lippan wrote: >>
> > Do you have svn? You could post patches from 'svn diff'. > See http://search.cpan.org/~timb/DBI/DBI.pm#CONTRIBUTING I am more of an SVK person, but svk is svn... kinda. I had the DBI tarball on my system and I would have had to go looking for the repository... I'll go look and set up a mirror. >> Oh, And is there a way to attach a string to an SV w/o copying it? > > Not sure what you mean by "attach a string to an SV". I'll guess you > mean append. In which case, yes, there are lots of ways: > Lets say I have a char *result. If I newSVpvn(*result, strlen(result)), *result is copied and I then have to free result... Is there a way to create a new SV without that copy? > >> --- DBI-1.604/DBI.xs 2008-03-24 09:44:38.000000000 -0400 >> +++ DBI-1.604-concat_hash/DBI.xs 2008-04-28 14:57:57.000000000 -0400 >> @@ -209,6 +209,95 @@ >> return buf; >> } >> >> +static int >> +_cmp_number (val1, val2) >> + const void *val1; >> + const void *val2; >> +{ >> + dTHX; >> + double first, second; >> + char **endptr = 0; >> + int old_err; >> + >> + old_err = errno; /* needed ? */ >> + errno = 0; >> + first = strtod(*(char **)val1, endptr); >> + if (0 != errno) { >> + croak(strerror(errno)); >> + } >> + errno = 0; >> + second = strtod(*(char **)val2, endptr); >> + if (0 != errno) { >> + croak(strerror(errno)); >> + } >> + errno = old_err; >> + >> + if (first == second) >> + return 0; >> + else if (first > second) >> + return 1; >> + else >> + return -1; >> +} > > Seeing this now I wonder if it might be better to have _sort_hash_keys > create an array of UV in parallel with an array of char* and then qsort > the array of UV if all the keys were valid numbers. > There would still need to be some way to tie the two arrays together after the sort, yes? Or just use the UV in the hash lookup? And what about floats? > That would avoid the frequent strtod() calls during sorting (and the > messing with errno) and avoid problems of accidentally doing a numeric > sort when not all keys are numeric. > I think the only time errno is set is for an under/overflow condition?... I could probably be ignored here. > > > You can use New() to avoid the need to test and croak: > > New(0, keys, hv_len, char *); I forgot about the error checking; I was thinking on going back and changing malloc's to New's to make it more xsish, but that is an even better reason to make the change. > >> + /* replace with function table */ >> + if (sort_order < 0) { > > You're assuming the 'char' type is signed. It might not be. > I see, I think. s/char/int/ >> + look_num = sv_2mortal(newSVpv(keys[0],0)); >> + if (looks_like_number(look_num)) >> + sort = _cmp_number; >> + else >> + sort = _cmp_str; > > I'd set sort_order = (looks_like_number(look_num)) ? 1 : 0; > here and change the 'else if' below to an 'if'. Simpler logic. > Okay. My plan was to change the 'else if's into a array of function pointers that that the sort_order indexed into, so to add a new sort order all you would do add an entry to the array of function pointers. >> + } else if (0 == sort_order) { >> + sort = _cmp_str; >> + } else if (1 == sort_order) { >> + sort = _cmp_number; >> + } else { >> + croak("Unknown sort order %i", sort_order); >> + } >> + qsort(keys, hv_len, sizeof(char*), sort); >> + return keys; >> +} >> +SV * >> +_concat_hash_sorted (hash, kv_separator, pair_separator, > value_format,sort_type) >> + HV *hash >> + SV *kv_separator >> + SV *pair_separator >> + SV *value_format >> + SV *sort_type >> + >> + PREINIT: >> + I32 hv_len; >> + STRLEN kv_sep_len, pair_sep_len, hv_val_len, pos=0, total_len = > 0; >> + char **keys; >> + char *joined, *kv_sep, *pair_sep, *hv_val; >> + unsigned int i = 0; >> + char sort; >> + SV **hash_svp; >> + SV *return_sv; >> + bool not_neat; >> + CODE: >> + >> + kv_sep = SvPV(kv_separator, kv_sep_len); >> + pair_sep = SvPV(pair_separator, pair_sep_len); >> + >> + if (SvGMAGICAL(value_format)) >> + mg_get(value_format); >> + not_neat = SvTRUE(value_format); >> + >> + sort = -1; >> + if (SvOK(sort_type)) { >> + sort = SvIV(sort_type); >> + } > > I'd use one line instead of four: > > sort = (SvOK(sort_type)) ? SvIV(sort_type) : -1; That works. I can go either way on the ternary operator. > > I also tend to add _sv suffix to SV variables in non-trivial subs that > have quite a few sv and non-sv variables (same for _av, _hv etc). E.g.,: > I don't see this, but if it make the code more DBIish, I go ahead and make the change. > sort_type = (SvOK(sort_type_sv)) ? SvIV(sort_type_sv) : -1; > >> + keys = _sort_hash_keys(hash, sort, &total_len); >> + if (!keys) { >> + ST(0) = Nullsv; >> + return; >> + } > > I think the no-keys case should return an empty string. Agreed. > >> + hv_len = hv_iterinit(hash); >> + /* total_len += Separators + quotes + term null */ >> + total_len += kv_sep_len*hv_len + > pair_sep_len*hv_len+2*hv_len+1; >> + joined = malloc(total_len*sizeof(char)); > > I think it would be better to keep appending to an sv rather than try to > pre-guess the size. That would avoid the need for the malloc/realloc/free. Well it would avoid my having to do it; Perl still does.... > Probably slightly more expensive, but not by much if you pre-grow the sv. But it also eliminates the copy of joined to an SV, so it might be a wash? > > The code would be simpler though. No need to keep track of pos, for > example. > Should boil down to something like this: > > value_string = (not_neat) > ? (SvOK(*value_svp) ? SvPV(*value_svp) : '') > : neatsvpv(*value_svp,0); > sv_catpvf(sv, (not_neat) ? "%s%s'%s'%s" : "%s%s%s%s", > keys[i], kv_sep, value_string, (i<hv_len-1) ? pair_sep : "") > > >> + for (i=0; i<hv_len; ++i) { > >> + hash_svp = hv_fetch(hash, keys[i], strlen(keys[i]), 0); >> + if (hash_svp) { > > hash_svp should always be true, so I'd make this a precondition check: > > if (!hash_svp) { > warn(...); > continue; > } > > and then you'd save one level of indent for the rest of the function. > (I don't like to see an indentation level going to waste :) Neither do I, I'll go fix that. -r
