Beefy Boxes and Bandwidth Generously Provided by pair Networks
Do you know where your variables are?
 
PerlMonks  

Re^12: ref to read-only alias ... why? (notabug)

by LanX (Saint)
on Jan 11, 2012 at 02:00 UTC ( [id://947271]=note: print w/replies, xml ) Need Help??


in reply to Re^11: ref to read-only alias ... why? (notabug)
in thread ref to read-only alias ... why?

Thanks for the detailed post!

I have to admit I'm not too much concerned about aliasing in for-each-loops, because they have a limited scope.

Loosing the overview about aliases in nested for-eaches implies bad coding practice resp. the need for refactoring.

So I'm more concentrating on the @_ aliases of subs, the possible effects are not limited to the sub.

Let's suppose a sub Package_A::trim($a) which uses call-by-reference to delete leading and trailing whitespaces.

Another sub Package_B::report() might create a report, and needs to trim the entries:

sub report { trim($_) for @_; return sprintf "...",@_; }

Some code in Package_C calls report() and passes occasionally a read-only value.

Now depending on the implementation of trim() the code might

a) die with "modification of read-only ..." or

b) continue but silently fail to delete the whitespaces.

That's why I think there should be at least a warning in case b.

> I don't see a point to a switch whose sole purpose is to break working code.

I don't understand why a warning would break existing code.

In general:

And I don't see a better coding practice, except avoiding subs like trim() using side-effects by manipulation $_[]

Clearly the author of report() doesn't expect read-only arguments, but since Perl5 doesn't have signatures like in Perl6 he can't easily enforce them.

He could rely on the implementation of trim which throws an error in case a). But after the author of Package_A switches to implementation b) of trim() the functionality of Package_B is broken.

And the different test-suites of packages A,B and C do not report any problem. I hope it's understandable now why aliasing in subs bothers me more than in for-eaches.

Cheers Rolf

PS: Sorry for the delay, quite busy ATM.

Replies are listed 'Best First'.
Re^13: ref to read-only alias ... why? (nowabug)
by tye (Sage) on Jan 11, 2012 at 07:04 UTC

    Your example code is just simply broken:

    sub report { trim($_) for @_; return sprintf "...",@_; }

    The idiom is my @strings = @_; and you can't just jettison that without some caveats. You've written report() that changes the data that you passed to it. That's unacceptable in any reasonable situation. "Reporting" should not modify.

    sub report { my @strings = @_; trim($_) for @strings; return sprintf "...", @strings; }

    Now, if you don't think about it much, your example code looks reasonable. But that's another reason why I don't write trim() that way. It looks like nothing is being modified.

    So the code is deceptive in appearance. If you had to write trim(\$_), then that obnoxious little \ character gives a little kick to your brain and might actually get you to realize that the code is doing something quite stupid: implementing 'print' that changes the stuff passed to it.

    Heck, even beyond the stealthy "modification at a distance with no visual cues" it also makes it a bit of a pain to use it in a way that doesn't modify things.

    # Doesn't work: my @strings = map { trimInPlace(\$_) } @_; # Works: my @strings = map { trimmed($_) } @_; my $msg = trimmed( shift @_ ); sprintf "...", map { trimmed($_) } @_; sprintf "...", trimmed($_[0]), ...;

    On the rare occasions when I have a routine that is likely to need to deal with huge sums of data, or that is going to be called frequently all over my code, then I might decide that my @strings = @_; might add up to enough overhead that I might even be able to notice it. So I might do extra work to allow a less-natural and more-error-prone interface option that does extra work to avoid a bunch of copying.

    But that is extra work and requires extra care. So I can't just throw some naive trim() at it. I have to do more thinking and realize that I can avoid the copy easily if no trimming is needed but I either need to copy or get tricky when I want to change one of the values I was given.

    At this point the simple example is stretched a bit too far so I'm going to skip trying to show some complicated code for this.

    So, if you use subtle tricks, then you often get burned (IME). And, if you use subtle tricks, then you are much more likely to run into things that behave subtly differently in different situations or with different builds of Perl.

    If "inconsistent" is unacceptable (and unconditionally implies "bug") to you, then you especially need to avoid subtlety.

    As for the proposed warning, I withhold judgement. At first I thought it was going to fire in tons of situations where a warning was unreasonable (and fixing that would be unacceptably expensive). I'm not convinced of that now but I'm not convinced it wouldn't ever do that, either. I'll let p5p chip in or maybe even decide to run the whole test suite and smoke test a bunch of CPAN and see when it fires and if it finds potential bugs or obnoxious false positives or what.

    And one guy saying "it is inconsistent and therefore a bug" and nobody agreeing and nobody feeling the need to fix the bug (or even comment) for 8 years looks to me like an abandoned argument, not a won argument. I can completely understand not wanting to try to explain against the simple-minded "inconsistent therefore bug" argument that it is simple minded.

    This is subtle stuff at this point and understanding subtle stuff requires effort from the person trying to understand. And I don't have the subtle details in hand about how threading causes the trade-offs to shift and resulted in a patch being applied specifically to make some behavior different (also known as "inconsistent") when threads are involved.

    About three steps removed from that, we come up with a confluence of subtle choices where the result is unfortunate. But I see this case as easy and wise to simply avoid by writing less-subtle, more-explicit code (and just fixing simply broken code).

    To push back against that choice, one must understand the subtle trade-offs for the threading case, of which I have not seen any description. I don't want to change it and suspect that I'd still feel that way after understanding it (and there is a lot that goes into that suspicion on my part but much of it is subtle and complex and also things that I would have to spend quite some time and effort to try to put into words). So I'm not investing the time to try to find that case and then to understand it. Sure, that's not proof. Neither is "it's inconsistent and one guy said 'bug' 8 years ago". Yeah, sometimes things really are that subtle and/or complex. *shrug*

    - tye        

Log In?
Username:
Password:

What's my password?
Create A New User
Domain Nodelet?
Node Status?
node history
Node Type: note [id://947271]
help
Chatterbox?
and the web crawler heard nothing...

How do I use this?Last hourOther CB clients
Other Users?
Others pondering the Monastery: (3)
As of 2024-03-19 04:18 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    No recent polls found