Re: Sort hash keys in C...

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 ;)

Changed.

>>     my @keys = keys %$hash_ref;
>>     no warnings 'numeric';
>>     my @keys = ($sort_type && $sort_guess)
>>         ? sort {$a <=> $b} @keys
>>         : sort    @keys;
> 
> Duplicate "my @keys" declarations.
> 
Fixed. And looking over the code, I also should go back and strictify...

> 
> No need for dTHX there.
> 

> or here.
> 

expurgated.

>> +char **
>> +_sort_hash_keys (hash, sort_order, total_length)
>> +{
> 
>> +    while ((entry = hv_iternext(hash))) {
>> +        *(keys+idx) = hv_iterkey(entry, &key_len);
>> +        tot_len += key_len;
>> +
>> +        look_num_sv = sv_2mortal(newSVpv(*(keys+idx),0));
>> +        if (1 == looks_like_number(look_num_sv))
>> +            *(numbers+idx) = (int)SvIV(look_num_sv);
>> +        else
>> +            numberish = 0;
> 
> You could avoid the expensive sv_2mortal(newSVpv()) by replacing those 5
> lines with something like this:
> 
>     if (grok_number(*(keys+idx), key_len, numbers+idx) ==
IS_NUMBER_IN_UV)
>         numberish = 0;
> 
> (The numbers array would need to be UV's, but that's no problem.)
> 

I like that. grok_number(): I must remember that one..


>> +    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?
 
>> +_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 *?

> 
> Thanks again for all your work on this Rudy!

No problem.  It has been a few years since I have done any C/XS coding, 
and I am trying to get back in the swing. Thank you for the suggestions.
> 
> p.s. I've already committed the sv_2mortal change in neatsvpv() - but in
> a different way, so you can drop that hunk from the patch before you
> apply the patch to the trunk.

I caught that after I applied it against trunk -- it took me a few minutes
to
figure out why I was getting double frees :)


-r