in reply to Re^7: Warnings on unused variables?
in thread Warnings on unused variables?

I'm deliberately conflating linear layout of code with the flow of the program, because that's what makes it easier to read. Exit points other than } are typically easier to notice, and can conceptually link "special thing happens here that I should watch for" with that point in a programmer's head.

I want a warning at the } if you have created an object that is going to do something special at that point and nothing has been done to make it visible. Call your destructor explicitly as a reminder, unless doing so would be even worse (i.e. you built a dozen of these objects in the same scope), in which case, put a single-line comment at the } and another at the line that turns off the warnings.

But as I said before, this is a stylistic thing, and I imagine it's not really worth getting into a big scuffle over it.

Replies are listed 'Best First'.
Re^9: Warnings on unused variables?
by Corion (Patriarch) on Sep 27, 2008 at 22:43 UTC
    Exit points other than } are typically easier to notice, can conceptually link "special thing happens here that I should watch for" with that point in a programmer's head.

    No. How many exit points are in this code:

    sub foo { my $resourceHandle = acquire_locked_resource(); my $dbh = get_database_handle; my $count = get_current_users($dbh); my $elapsed = get_elapsed_time(); my $users_per_second = $count/$elapsed; $resourceHandle->output("$users_per_second users/s"); $resourceHandle->release; };

      Assuming that you haven't deliberately made things worse on yourself by concealing objects, a programmer new to your code will note that you have kindly warned him that $resourceHandle is an object via the ->release just before the closing brace (this is the thing that I want the warning to check for, and you've done exactly that), and check acquire_locked_resource, get_database_handle, get_current_users, get_elapsed_time, and the output method on whatever acquire_locked_resource claims in the comments to return, and assume that you were sane enough to make the release an explicit destructor before the brace. If you did something insane like sticking an object in $count and $elapsed and hid an operator overload in '/', or hid an object in $users_per_second and then tinkered with the lvalue assignment, then committed that sub without comment, you deserve to have your access revoked.

      In my stylistic opinion, of course. No personal offense or disrespect intended, especially given that this is just example code.

      That said, the original topic at hand is warning messages on unused variables, and you've declared: $resourceHandle, $dbh, $count, $elapsed, and $users_per_second, and every single one of those got used. Why are you worrying about the warning on this code?

        I have to entirely disagree with you here. I *like* set-and-forget semantics. (I'm actually concerned about mark-and-sweep garbage collection in Perl 6 going toward's Java's non-guarantee of destructor calls, though I don't really know where Perl 6 stands on it.) It means that the compiler will generate code that properly handles resource reclamation, whether that's releasing a database connection (and rolling back any transactions currently underway), or deleting a temporary file (talk about security holes!), or decrementing a lock counter (possibly freeing it). It means that I can't forget to do the cleanup. And I forget a lot.

        Missing a "release" on a lock, for example, can lead to deadlock. Missing database connection releases can result in wasted server resources in tracking unfinished transactions, possibly reducing responsiveness on the server. Failing to delete temporary files can result in disk space consumption, often on partitions that are among the smallest partitions on the box if on unix, possibly exposing old data to users that shouldn't see it (though leaving it open to other users for *any* length of time is bad, but auto-delete can reduce the scope of the vulnerability). These can be catastrophic errors. Manually freeing resources that can be freed automatically is just asking for problems. Letting the compiler do it for you is Lazy - in the right way.

        This is entirely about "$resourceHandle->release; };" being clearer than "};"

        {Warnings to enforce style}--

        My example is not about "omitting the final explicit destructor", it's about forgetting the final explicit destructor. That code block can die in at least two places ($dbh set to RaiseError, or $elapsed being zero) and then never call the destructor, leading to memory leaks and/or unreleased locks. Creating the lock-release right at lock acquisition prevents such oversights, and having to disable each and every such instance is foolish.