I know variants of the following bug have been discussed in the Monastery recently, but I didn't find this particular one. My apologies in advance if I missed it.

Spot the bug:

sub foo {
  my $hashref = shift;
  while ( my ( $key, $value ) = each %$hashref ) {
    return if sometest( $key );
    frobnicate( $key, $value );
  }
}

[download]

BTW, there's nothing arcane about this bug. It's vanilla Perl, but it's easy to miss.

This is yet another guise of the ol' each bug. The problem is that if the function is called with the same arguments twice, and if during the first call the while is exited due to the result of sometest , then in the second call to the foo, the iteration will not begin in the same place as it did in the first call. Conceivably this could be the desired behavior, but I bet that in most cases it isn't.

To fix it, use keys before the loop: 

sub foo {
  my $hashref = shift;
  keys %$hashref;
  while ( my ( $key, $value ) = each %$hashref ) {
    return if sometest( $key );
    frobnicate( $key, $value );
  }
}

[download]

Update: The original title of this thread ("Buglet") was confusing, so, by popular demand, I changed it. I hope the new title is an improvement.

the lowliest monk

Replies are listed 'Best First'.
Re: Spot the bug
by merlyn (Sage) on Dec 21, 2005 at 01:54 UTC
[reply]

      By "bug", I presume you mean "bug in your code". The "exiting from the middle of an each-loop" has been discussed here before, and the behavior of Perl is as documented.

      Sorry I gave the impression (probably through my choice of title) that I thought this was a bug in Perl. I did mean a bug in the snippet I presented. Perl's behavior is, as you say, amply documented.

      the lowliest monk

[reply]
      And for small hashes, that is fine. It's less so for a general purpose function in a module - it might get passed in a large hash, possibly even one that's tied to storage on disk.
      Perl --((8:>*
[reply]

        The problem is people have a bad habit of forgetting to reset the iterator, and heaven help you if the general routine calls back out to uncontrolled code from inside the loop. Can you say "infinite loop"? :-)

        ---
        $world=~s/war/peace/g

[reply]
        Well, I'd argue that any program for which the difference between a keys-based walk and an each-based walk is significant shouldn't be using hashes that large, not with the arrival of things like DBD::SQLite or other cheap databases.

        Yes, it mattered when Perl was running on 10Mhz 1Megabyte machines, and databases were tough, so a DBM hash was a cool thing. Those are long gone. {grin}

        -- Randal L. Schwartz, Perl hacker
        Be sure to read my standard disclaimer if this is a reply.

[reply]
Re: Spot the bug
by ysth (Canon) on Dec 21, 2005 at 19:16 UTC
[reply]
[d/l]

Back to Meditations