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
