in reply to RFC - inplace upgrade for Tie::SortHash

I cant help but wonder if your use of autovivification to create the various elements of the object is dangerous. I wouldnt do it personally. I have a feeling at least some of your code will die if somebody calls the wrong function at the wrong time. For instance, when I turn warnings on (why arent you using them?!) the following snippet produces warnings and also doesnt work as expected: 

tie my %hash,'Tie::SortHash',{a=>1,b=>2};
delete $hash{'x'};
print join "\t",keys %hash;

[download]
Use of uninitialized value in splice at D:\Temp\tie_sorthash.pl line 4
+3.
b

[download]

Where did the 'a' key go? Also delete() doesnt set the CHANGED flag, and will not play nicely with keys. (It should always be possible to delete the last visited key while iterating the hash without disrupting the hash.) 

sub _ReOrder {
    my $self = shift;
    $self->[LOOKUP] = ();
    $self->[ARRAY] = ();

[download]

If you intend to set these two to undef then I can think of more obvious ways. Otherwise this is a bug. 

 defined $self->[ARRAY][$index] ? $self->[ARRAY][$index] : undef;

[download]

What exactly is the purpose of this line? Isnt the conditional totally redundant?

I think you need to revisit the code and build up a big set of tests. I have a feeling there are other whammies in there as well. Another question I have is why the iterator requires a hash lookup each time?


---
demerphq

<Elian> And I do take a kind of perverse pleasure in having an OO assembly language...

Replies are listed 'Best First'.
Re: Re: RFC - inplace upgrade for Tie::SortHash
by Limbic~Region (Chancellor) on Sep 05, 2003 at 04:51 UTC
    demerphq,
    I really appreciated your comments. I haven't touched this for almost a month. I was waiting on the module author to return my emails, which did not happen. Today I decided to start over. I would like to address the points you made on the previous code:

  • I cant help but wonder if your use of autovivification to create the various elements of the object is dangerous
    The code creates a new hash from an old hash. It is only dangerous to the extent that I trusted that what I was being passed was a hash reference without testing it.

  • Why arent you using warnings?!
    The warnings were not turned on because the original module author did not use them.

  • Where did the 'a' key go?
    Code I added to optimize the iteration created an array with the hash keys in order. I figured another optimization would be to delete the array element without going through the sort routine again. The problem was that I didn't first verify that the key being deleted actually existed. The 'a' disapeared because it happened to be the 0 index - the result of an undefined variable

  • Also delete() doesnt set the CHANGED flag, and will not play nicely with keys. (It should always be possible to delete the last visited key while iterating the hash without disrupting the hash.)
    Correct - it doesn't set the changed flag. It accomplishes the same thing by deleting the array element described above. I do not believe there would be any disruption with deleting a key while iterating other than your discovery of deleting a key that doesn't exist.

  • If you intend to set these two to undef then I can think of more obvious ways. Otherwise this is a bug. 
    defined $self->[ARRAY][$index] ? $self->[ARRAY][$index] : undef;

What exactly is the purpose of this line? Isnt the conditional totally
+ redundant?

    [download]

    The code is correct as written. The conditional is designed to detect when you have fallen off the end of the array and return undef. I am not sure what is more obvious in undef'ing the arrays then what I used?

  • I think you need to revisit the code and build up a big set of tests. I have a feeling there are other whammies in there as well.
    I indeed did find lots of whammies today when I went back over the code. A few I introduced when I tried to "fix", in my opinion, very bad code. The rest were already there.

  • Another question I have is why the iterator requires a hash lookup each time?
    I use an array with the hash keys in order. The problem is that I am only aware of the last key. I use the hash lookup to get the index of that element in the array. Getting the next key is then a simple matter of adding 1.

    Later, when I have time to build up a test suite and finish the POD, I will re-post an RFC that outlines my questions and concerns. In the interim, here is the new non-backwards compatible code.

    Read more... (5 kB)
    Cheers - L~R
[reply]
[d/l]
[select]

      Today I decided to start over. I would like to address the points you made on the previous code:

      Hmm, im not up for responding completely yet, so you can expect a /msg that this reply has been updated. However, there is one point id like to address:

      The code is correct as written. ...

      defined $self->[ARRAY][$index] ? $self->[ARRAY][$index] : undef;

      [download]

      Except that its misleading as hell. It is _exactly_ equivelent to 

      $self->[ARRAY][$index]

      [download]

      Except that it makes a reader wonder what magic specialness is going on, and is marginally slower. If nothing special is happening then it should be changed to the latter IMO


      ---
      demerphq

      <Elian> And I do take a kind of perverse pleasure in having an OO assembly language...
[reply]
[d/l]
[select]
        demerphq,
        I have to disagree. 
        defined $self->[ARRAY][$index] ? $self->[ARRAY][$index] : undef;

        [download]
        Is equivalent to: 
        if (defined $self->[ARRAY][$index]) {
    return $self->[ARRAY][$index];
}
else {
    return undef;
}

        [download]
        Cheers - L~R
[reply]
[d/l]
[select]

In Section Meditations