On Fri, May 16, 2008 at 01:45:51AM +0100, Rudolf Lippan wrote:
> On Thu, 15 May 2008 13:21:34 +0100, Tim Bunce <[EMAIL PROTECTED]> wrote:
> > I'm sorry for the delay in responding Rudy.
> >
> > On Wed, May 07, 2008 at 02:34:43AM -0400, Rudy Lippan wrote:
>
> >> use Scalar::Util qw(looks_like_number);
> >> sub _get_sorted_hash_keys {
> >> my ($hash_ref, $sort_type) = @_;
> >> my $sort_guess = 1;
> >> $sort_guess = 1!=looks_like_number($_) ? 0:$sort_guess for keys
> > %$hash_ref;
> >> $sort_type = $sort_guess unless (defined $sort_type);
> >
> > You don't need to calculate $sort_guess unless $sort_type is undef.
>
> I know. The code is only used for testing against the C implementation, and
> putting it inside the #if defined($sort_type) made the code wrap ;)
:-)
I only really mentioned it because you'd included a benchmark and that
change would improve the pure-perl performance.
> >> + if (0 == sort_order || 0 == numberish ) {
> >> + qsort(keys, hv_len, sizeof(char*), _cmp_str);
> >> + } else {
> >> + qsort(numbers, hv_len, sizeof(int), _cmp_number);
> >> + for (idx = 0; idx < hv_len; ++idx)
> >> + *(keys+idx)
> > =SvPV(sv_2mortal(newSViv(numbers[idx])),key_len);
> >
> > It would be good to add a clear comment somewhere that the pointers in
> > the keys array are only valid until perl next free mortal temps.
> > (There are faster ways of doing that but the use case for the numeric
> > keys isn't speed critical.)
> >
> Suck as, if you don't mind me asking?
("Such as", I presume :)
You could realloc the keys buffer to grow it by the size of all the key
strings plus null bytes. Then write them into that extra space.
Or, more deeply, instead of sorting an array of int's you could sort an
array of { int key_as_int, char *ptr_to_key } structs. That way you'd
also avoid the potential risk of problems with keys like "01" or "1.0"
that change when converted to int and back to a string.
> >> +_concat_hash_sorted (hash, kv_sep_sv, pair_sep_sv,
> > value_format_sv,sort_type_sv)
> >> + PREINIT:
> >> + CODE:
> >
> > I'd be grateful if you could refactor this so the bulk of the code is in
> > a C function and the XS is just a small wrapper. Would make it easier to
> > use for the ShowErrorStatement code (around line 3387).
>
> Sure. leave params to the C function as SV *?
Whatever seems reasonable (and best for ShowErrorStatement usage).
Thanks!
Tim.
