tchrist@convex.COM (Tom Christiansen) (03/14/91)
From the keyboard of cwilson@NISC.SRI.COM (Chan Wilson [Animal]):
:While finishing up my user interface part to a label printing program,
:I came up with the following piece of strangeness, which i believe is
:part of the next subroutine bug described by others..
Um, no; I really don't think so, but the code is confusing enough to my
top-down, simplistic, straight-forward brain* that I wouldn't swear to
this. I think you're guilty of scoping abuse; maybe other kinds as
well. Let's see...
[This is a bit of a code walk-through again, and i don't want anyone
to take offense at it this time. I'm just trying to help.]
:What I was trying to do was insure that the user really gave an answer
:to the question, so I wrote the following subroutines:
:
:sub input {
: $_[0] = <STDIN>; chop $_[0]; return (length($_[0]));
:}
Ok, so input will modify its parameter. This is generally considered
harmful. Notice the very strong warning on page 100 of the book: (using
underlining for bold font):
Passing Arguments by Value
Since a subroutine is a BLOCK, you can create local variables
using the _l_o_c_a_l function (see Chapter 4). Use array assignment to a
local list to name your format arguments:
sub maybeset {
local($key, $value) = @_;
$foo{$key} = $value unless defined $foo{$key};
}
Assigning your parameters to a local list also has the effect of
turning call-by-reference into call-by-value, since the assignment
copies the values. _W_e _r_e_c_o_m_m_e_n_d _t_h_a_t _y_o_u _a_l_w_a_y_s _d_o _t_h_i_s _e_x_c_e_p_t _i_n
_e_x_c_e_p_t_i_o_n_a_l _c_a_s_e_s. While call-by-reference is efficient, it makes for
some interesting aliasing problems that novices could just as soon do
without, especially in combination with the dynamic scoping provided by
the local operator.**
** If you understood all that, then you're allowed to use
call-by-reference anyway you want.
If that's not strong enough warning, I dunno want is. If you add in the
complications caused by the issues of @_ being sometimes set and sometimes
not set, and the call-by-name stuff (that you haven't used -- thank god),
scoping can be very twisted. Do try to use local() for all your stuff.
(By the way: those parens on the return are no more needed in perl than
they are in C. Of course, in perl, you don't even need the return
keyword.)
I think it would have been cleaner to make this work this way:
sub input {
local($_) = scalar(<STDIN>);
chop;
$_;
}
and called it this way
$foo = &input;
:sub Bzzzt {
: print stderr "\n\007<Bzzzt> I need an answer here...\n\n";
:}
ok, looks fine.
:and then tried to write code like so:
:
:
:sub WhatToPrint {
: print 'Whatcha after?
:
:1) Ring Labels
:2) Side Labels
:3) Ring and Side Labels
:
:==>';
:
: &input($labeltype) || {&Bzzzt; &WhatToPrint;}
: ^^^^^^^^^^^^^^^^^^^^^^^^^ offending part
:}
:
:Naturally, I discovered that I can't do that.
Right -- because that's not in the language's syntax, versatile though it
may be. Here are some alternatives:
unless (&input($labeltype)) {
&Bzzzt;
&WhatToPrint;
}
or
&Bzzzt, &WhatToPrint unless &input($labeltype)
or
&input($labeltype) || do {
&Bzzzt;
&WhatToPrint;
};
or
&input($labeltype) || (&Bzzzt, &WhatToPrint);
Of course, the truth is that I would have written it more this way:
while ($labeltype = &input) {
&Bzzzt;
&WhatToPrint;
}
You still aren't catching what happens when the user types EOF, but
that's another story.
:So I rewrote a few things:
:
:sub Bzzzt {
: print stderr "\n\007<Bzzzt> I need an answer here...\n\n";
: do $_[0];
:}
Whoa, there! What's that $_[0] there??? Whose is it? Certainly Bzzzt
didn't have any params passed to it, so we have to look up the call
chain. I can't find one. It depends on your own program. Find the first
guy who called him with a parameter. That's his $_[0]. I doubt you
wanted that.
But there's more here: you said
do $scalar;
which is NOTHING LIKE
do $scalar();
The first is kind of like (modulo @INC etc):
eval `cat $scalar`;
that is, it sources a file, whereas the second is more like
&$scalar();
to call an indirect function.
:sub WhatToPrint {
:[.....]
: &input($labeltype) || &Bzzzt(WhatToPrint);
:
:
:which had the _very_ interesting aspect of re-entering WhatToPrint at
:the next executable line. It looks like this is another facet of the
:next bug.
Ok, I see -- maybe you only called Bzzzt like this. But I doubt very much
that you should happen have a file named 'WhatToPrint' lying around, so
that "do" didn't do very much.
While we're on it, I personally consider it bad practice to use
unquoted string constants. I may relax a little on the
(Monday,Tuesday,Wednesday,Thursday,Friday,Saturday,Sunday)[$day];
case, but even makes me a tad nervous. If you want to pass the subroutine
name, do please quote it. (I won't even suggest pass-by-name. That would
be truly horrific.) They look too much like filehandles to me.
:But the elegent (IMHO, naturally) thing is what I came up with next.
:I restored Bzzzt to original, and without really thinking, re-wrote
:WhatToPrint to say:
Elegant??? Well, I guess you did say it was your opinion. I don't
think I'd say quite the same.
:sub WhatToPrint {
:[.....]
: &input($labeltype) || &Bzzzt && &WhatToPrint;
:which reads as "Do sub input with $labeltype. if 0 is returned, do
:Bzzzt _AND_ do WhatToPrint", which is what I intended, but certainly
:not what perl is interpreting it as.
No, I don't think it reads this way. If you meant that, you would have said:
&input($labeltype) || (&Bzzzt, &WhatToPrint);
What you said was equivalent to:
if (! &input($labeltype)) {
if (&Bzzzt) {
&WhatToPrint;
}
}
which is quite different.
And you're recursing, no less! Gosh, is that really needed? Wouldn't
a simply redo work?
:Interesting, no? Seems to me this is a way around the `mumble || {foo}'
:restriction...
:btw, why is that restriction there in the first place? conflict with
:the 'real' use of || ?
I think I've explained that, but here it is one more time in different
words. An piece of an expr cannot a BLOCK be. The {} starts a block.
You can, however, have a piece that is "do { }" instead, and the value of
the block will be that of the last value evaluated in that block, just as
it is with a subroutine.
Here, by the way, is really how I might code that:
exit 1 unless defined ($labeltype = &input);
sub input {
local($_);
{
last unless defined($_ = <STDIN>);
if (/^\s*$/) {
print "null input: \007please select one of ...\n";
redo;
}
chop;
}
$_;
}
--tom
*You might not agree with this description if you look at my
more serious coding endeavors. You'd probably be right.