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

Mhm, either you and I have different definitions of 'accurate', or we have different conceptions about what constitutes a useful warning. I want warnings on code that, especially if handed to someone who didn't write it, is more likely than equivalent code that doesn't generate the warning to become the seed of a future bug. To me, a warning says, "Okay, well, yes, you can technically do it that way, but it's likely to be better in the long run if you do something else."

Specifics follow the jump.

#2 ignores one of the function's return values without making that fact explicit to a casual viewer at the time the function is called. We may have a difference of style here, but to me, that's wrong. If you're going to discard, use undef.

#3 indicates a position where a filehandle was used for an open and never used again, not even to check return value on a close. For the trivial example you posted, where no data is ever written, this is fine, and I can make it explicit (and avoid the warning) by simply closing the file before the braces end. It adds all of a dozen characters, and leaves the warnings valid for any time I had opened multiple files, and then accidentally used one of the file descriptors twice in a row instead of using the one I intended.

#6 isn't required to make an inside-out object; it's a shortcut for making an anonymous scalar instead of an anonymous hash, and I've never had it explained to my satisfaction why it is substantially better than an anonymous hash, which by removing a named variable at least makes it clear that the variable isn't intended to actually hold anything.

On to the other class of examples, I just tested that #1 in fact does use $foo++, and warnings::unused handles it properly:

#!/usr/bin/perl use warnings; use warnings::unused; use strict; my $foo = 42; my $bar = 43; exit(eval q[$foo++]);
prints:
Unused variable my $bar at ./warntest.pl line 8.

#4: I misunderstood your example, my apologies, but now that I think I see what you're getting at, I'm not sure why putting your locks in your constructor (and by the way, although again this is a matter of style, I would not personally use constructor methods named in such a way that they can be easily mistaken for procedures) is substantially better than instantiating a new object containing the resources you want unlocked and then explicitly locking the object until it passes out of scope. The latter seems clearer, and I would not mind a warning for that reason alone.

#5: Again, I misunderstood what was being done... but I'd argue that this is exactly why I want a warning on code that looks like that. You've hidden functionality inside a destructor-only event that is going to trigger many lines of code away from when you prepped it. I want a warning if someone does that. You can lose the warning by explicitly undef'ing the object before it goes out of scope, which makes it very clear that an object going out of scope is about to do something significant other than going away.

I guess the short version of this is, I don't see any situation where using a declare-once-and-forget variable is ever clearer or substantially more efficient than an alternative, so I don't see any of these situations as warranting exceptions from warnings, even if they are in common use.

Replies are listed 'Best First'.
Re^5: Warnings on unused variables?
by Corion (Patriarch) on Sep 27, 2008 at 21:23 UTC
    I just tested that #1 in fact does use $foo++, and warnings::unused handles it properly

    You forgot about tied variables. $foo++ can be tied to code that prints out "Hello World" as a side effect.

    I guess the short version of this is, I don't see any situation where using a declare-once-and-forget variable is ever clearer or substantially more efficient than an alternative, so I don't see any of these situations as warranting exceptions from warnings, even if they are in common use.

    You haven't programmed much with resource acquisition/release systems then. C has lots of problems with that, Windows COM has lots of problems with that and Perls reference counting also has these problems (although many of them have been fixed in all instances). With Reference Counting as the instance of resource acquisition and release, you have exactly the problem that the points of acquisition (increase of refcount) and release (decrement of refcount) are not in the same place of the code and hence, quite unpleasantly, not every acquisition is matched with a release, because it's hard to see that they match up because they do not happen in the same place. Which is why a "execute this code at scope exit" functionality is wanted.

      That's the point of resource acquisition/release, sure. Why is it clearer to allow it to silently happen at the closing brace than to explicitly release on the line before the closing brace? The issue isn't whether or not you've used a destructor with a special ability — it's whether you've done so in a way that isn't clear, and thus should trigger a warning.

      Also, does a tie matter? Would a tie have caused $foo++ cause warnings::unused to throw a warning?

      (I posted this comment earlier, but it seems to have disappeared. Apologies if this makes it show up twice.)

        You are conflating the linear layout of your code with the flow of your program, which is not necessarily linear. If you can specify what should happen at scope exit, you don't need to specify that at the end of the scope, especially if the scope can be left through various exit points, like die for example. And it's even less a matter of where such things are specified and more a matter of that they're specified and executed at all, for all exit conditions. And hence, it's easier and IMO even clearer to specify what should happen at cleanup time (whenever that is) in the same place where the resource is acquired.

Re^5: Warnings on unused variables?
by ikegami (Patriarch) on Sep 27, 2008 at 22:58 UTC

    #3 indicates a position where a filehandle was used for an open and never used again, not even to check return value on a close.

    close will never fail, cause all we want to do is create the file.

    It adds all of a dozen characters

    But that's a dozen of characters of unnecessary code. For just one line of code! I'd have useless code all over the place. Code that can do nothing but introduce errors, since it doesn't serve any function.

    and leaves the warnings valid for any time I had opened multiple files, and then accidentally used one of the file descriptors twice in a row instead of using the one I intended.

    No. Since you believe in closing everything explicitly, the warning won't help you there.

    # $fh1 $fh2 # ---- ---- open(my $fh1, '>', $file1) or die; # 1 open(my $fh2, '>', $file2) or die; # 1 for (@data) { # print $fh1 "$_\n"; # 1 print $fh1 "$_\n"; # Oops # 1 } # close($fh1); # 1 close($fh2); # 1 # ---- ---- # 4 2 -> no warnings

    Furthermore, autodie is a great module for testing errors on close, and it doesn't reference the file handle explicitly.

    I'm not sure why putting your locks in your constructor

    No, the constructor obtains the lock. The object is the lock.

    I would not personally use constructor methods named in such a way that they can be easily mistaken for procedures

    You've never used threads? And constructors are routinely called names other than new.

    I'm not sure why [RAII] is substantially better [...] explicitly locking the object until it passes out of scope.

    To properly release the lock when an exception occurs or when return is used more than once.

    but I'd argue that this is exactly why I want a warning on code that looks like that.

    I'd rather avoid doing away with exceptions, I'd rather my code doesn't throw spurious warnings, and I'd rather not have to us no warnings; all over the place. That's why it's better checked by a linter or until turned on explicitly (meaning -v and use warnings; wouldn't enable it).

    You've hidden functionality inside a destructor-only event that is going to trigger many lines of code away from when you prepped it. I want a warning if someone does that.

    You say deallocation of resources on block exit worthy of a warning? Every single variable in Perl does that!

      close will never fail, cause all we want to do is create the file.

      In this situation, yes. This may not be true later.

      But that's a dozen of characters of unnecessary code. For just one line of code! I'd have useless code all over the place. Code that can do nothing but introduce errors, since it doesn't serve any function.

      No, you shouldn't need it more than once, because that's a corner case you can stick in a procedure. The final close serves the function of making missing filehandles visible elsewhere.

      Since you believe in closing everything explicitly, the warning won't help you there.

      Careful, that's not what I said. I believe in making sure that every variable that was intentionally not used gets used in such a way that it is marked, even via an unnecessary close call, exactly because it allows you to catch errors like this by turning on warnings::unused:

      I'd rather avoid doing away with exceptions, I'd rather my code doesn't throw spurious warnings

      Well, we obviously have a fundamental philosophical disagreement about what is spurious. It may not be important; I'm not sure a discussion of warning philosophy warrants exclamation points and so much node space, but I'll offer you this: if you can show me a code formation that is plausibly frequently found in a project, raises warnings under warnings::unused, and any changes that would result in the warnings going away also results in the code becoming significantly less clear or maintainable, I'll concede the point and try not to bring it up again. We can play code tennis: you post something that warns, but is otherwise clear, and I try to make it clearer and warning-free. Writing code can be fun (and I do intend to follow up on your question about the behaviour of warnings::unused in different scenarios, because it is code). This kind of essay isn't, really, so otherwise, since I'm obviously not in a position to mandate anything in the development of Perl 6, let's leave it at that.

      <tweetybirdvoice>Twoo-Love!</tweetybirdvoice>

        No, a simple deallocation of resources wouldn't have required you to lodge everything in a constructor/destructor that way.

        Sorry, but that's all that those two examples did.

        You're refcounting from copies made in methods, or doing something else subtle

        Really, no. One released OS resources, the other released Perl resources by breaking cyclic references.

        Well, we obviously have a fundamental philosophical disagreement about what is spurious

        That's been my point all along. You're enforcing style, and thus a warning turned on by -w or use warnings; is not appropriate.

Re^5: Warnings on unused variables?
by ikegami (Patriarch) on Sep 27, 2008 at 23:02 UTC

    In to the other class of examples, I just tested that #1 in fact does use $foo++, and warnings::unused handles it properly:

    That's odd.
    Does eval '$'.'foo++' issue a warning for $foo? (It shouldn't)
    Does eval 'print q{$foo++}' issue a warning for $foo? (It should)
    Does eval '$cond ? $foo : $bar' issue a warning for $foo and $bar? (It shouldn't)

      Just tested, using the same code I used earlier, substituting in each new eval block one at a time without using exit(), i.e.:

      #!/usr/bin/perl use warnings; use warnings::unused; use strict; my $foo = 42; my $bar = 43; eval '$'.'foo++';
        Turns out it was easy to install on my Windows machine. It stops behaving "properly" when the eval is in a function. Warnings for both $foo and $bar are emitted as soon as the function is compiled.
        #!/usr/bin/perl use warnings; use warnings::unused; use strict; BEGIN { print("1\n"); } sub foo { my $foo = 42; my $bar = 43; eval '$foo++'; print("4\n"); <>; # <-- Segfaults when this is executed. } # <-- Warns when this is compiled BEGIN { print("2\n"); } print("3\n"); foo(); print("5\n"); <>;
        Interesting! It must produces when the program exits. The existing warning for package variables occurs at compile-time.