faulty expression prob

perl-diddler has asked for the wisdom of the Perl Monks concerning the following question:

I had a relatively simple situation where I wanted to assign one of 2 values to an variable and return the result. I thought to use a conditional return:

return ( ($rh=$pkg->{'RPM_HDR'}) and $dist=$rh->tag('DISTRIBUTION') )
    ?    $pkg->{'DIST'}=$dist 
    :    $pkg->{'DIST'}=$undef;
[download]

But it doesn't work. Less elegant alternatives work like:

( ($rh=  $pkg->{'RPM_HDR'}) and $dist=$rh->tag('DISTRIBUTION')) and
    return $pkg->{'DIST'}=$dist;
return $pkg->{'DIST'}=$undef;
[download]

And the verbose, very steppable:

if ($rh=$pkg->{'RPM_HDR'}) {
    if ($dist=$rh->tag('DISTRIBUTION')) {
        $pkg->{'DIST'}=$dist;
        return $dist;
    }
}
return $undef;
[download]

I thought the conditional return expressed what I wanted the best, but it ends up return "$undef" when it should be returning "$dist". I.e. "$pkg->{'RPM_HDR'}" contains a pointer to an rpm_header, and $rh does have a valid tag for "DISTRIBUTION". Is there a reason why (this is in 5.8.8, BTW), why it wouldn't? As I understand the return's expression, if should 1st do the assignment to $rh (and test the result for defined & nonzero). If that is true, then it should do the 2nd part after then 'and' -- so that "should" assign the tag from $rh to $dist -- it is also defined and nonzero. So, since true, eval 1st expression and return it (the line containing 'dist'. So what am I missing -- why doesn't the more 'attractive' one work? *sigh* TIA...

Comment on faulty expression prob Select or Download Code

Replies are listed 'Best First'.
Re: faulty expression prob by BrowserUk (Patriarch) on Apr 03, 2008 at 03:34 UTC
I'm not squemish about compound statements, and I like to avoid unnecessary code, but this makes me wince: `return $pkg->{'DIST'} = ( $rh = $pkg->{'RPM_HDR'} and $dist = $rh->tag('DISTRIBUTION') ) ? $dist : $undef;` [download] It should work, but since you need two temporary vars which have to be declared (you do use strict don't you;), then this seems much cleaner: `$pkg->{'DIST'} = undef; my $rh = $pkg->{'RPM_HDR'} or return; my $dist = $rh->tag('DISTRIBUTION') or return; return $pkg->{'DIST'} = $dist;` [download] Probably minutely more efficient to boot. Examine what is said, not who speaks -- Silence betokens consent -- Love the truth but pardon error. "Science is about questioning the status quo. Questioning authority". In the absence of evidence, opinion is indistinguishable from prejudice. "Too many [] have been sedated by an oppressive environment of political correctness and risk aversion."	[reply] [d/l] [select]
Re: faulty expression prob by jwkrahn (Abbot) on Apr 03, 2008 at 03:27 UTC
This should work: `return $pkg->{ DIST } = ( $rh = $pkg->{ RPM_HDR } and $dist = $rh->tag +( DISTRIBUTION ) ) ? $dist : $undef;` [download]	[reply] [d/l]
Re^2: faulty expression prob by perl-diddler (Chaplain) on Apr 03, 2008 at 23:07 UTC
That's were I was going (where I wanted to end up) as I was trying to remove the duplication in the code. The 2 mentions of the L-value "$pkg->{DIST} were definite candidates for reduction, but I got stuck along the way when the less 'reduced' expression I initially posted wasn't working as expected...Perhaps someone can point out the "logical error" from the following change of "logic" (I'm paying careful attention to op-precedence as others have suggested that might be the problem). BTW, "?:" has higher precedence than assignment "=". "and" is lower than both. From the orig (dropping unnecessary parens around assign before 'and' (numbering for greater easy in referring back to which step is causing problem) (0): `return ( $rh=$pkg->{'RPM_HDR'} and $dist=$rh->tag('DISTRIBUTION') ) ? $pkg->{'DIST'}=$dist : $pkg->{'DIST'}=$undef;` [download] Logically substituting "X" for the conditional expression in parens, "( $rh=$pkg->{'RPM_HDR'} and $dist=$rh->tag('DISTRIBUTION') )", I get (1): `return X ? $pkg->{'DIST'}=$dist : $pkg->{'DIST'}=$undef;` [download] AHAH!....I see the prob...that "=" on the "end" (before $undef)... It is lower precedence than the ?: (as mentioned above...but wasn't sure which '=' the '?:' was interfering with). So the above (using D for "$pkg->{'DIST'}) is really (2?): `return (X ? D=$dist : D ) = $undef;` [download] Um...Why wouldn't that be a syntax error? the "(X?D=$dist:D)" part isn't a valid address that can be assigned to. (Note. "$undef" != undef, it's equal to a string of length 5 characters, 'undef'). I can see why what I wrote would be wrong -- but why wouldn't Perl flag it as illegal syntax?	[reply] [d/l] [select]
Re^3: faulty expression prob by jwkrahn (Abbot) on Apr 04, 2008 at 02:41 UTC
Why wouldn't that be a syntax error? Because ?: can be used on either the right hand side of an assignment: `my $var = $test ? $value_one : $value_two;` [download] Or on the left hand side of an assignment: `( $test ? $var_one : $var_two ) = $value;` [download] And because `$pkg->{'DIST'}` can be assigned to it is not a syntax error.	[reply] [d/l] [select]
Re: faulty expression prob by bobf (Monsignor) on Apr 03, 2008 at 03:30 UTC
I suspect you are running into a precedence issue, specifically with the conditional operator (?:) and the assignment operator (=). See perlop. Personally, though, I found the code hard to read. I'd probably write it something like this: `$rh = $pkg->{'RPM_HDR'}; $dist = $rh->tag('DISTRIBUTION'); # or $dist = $pkg->{'RPM_HDR'}->tag('DISTRIBUTION'); $pkg->{DIST} = $dist ? $dist : $undef; # $undef or undef?` [download] Update: Given how you are testing for truth, keep in mind what Perl considers to be a true value (vs a defined value). See True or False? A Quick Reference Guide for examples.	[reply] [d/l]
Re^2: faulty expression prob by ikegami (Patriarch) on Apr 03, 2008 at 07:55 UTC
You removed the check that prevented the method `tag` from being called on an undefined value. `$dist = $rh->tag('DISTRIBUTION');` [download] should be `$dist = $rh && $rh->tag('DISTRIBUTION');` [download] Also, `$pkg->{DIST} = $dist ? $dist : undef` [download] can be written as `$pkg->{DIST} = $dist \|\| undef` [download]	[reply] [d/l] [select]
Re^3: faulty expression prob by perl-diddler (Chaplain) on Apr 03, 2008 at 23:17 UTC
Um...somewhere along the line, someone dropped a "$" before "undef"..."$undef" != undef, but $undef = 'undef''; In answer to next Q, I did want assignments -- they needed to be evaluated or done left-to-right, where the 'right' side isn't done if the left side fails... If I wanted to use "==" instead of assignment, I would have preferred '&&' over 'and'. I try to use 'and' && 'or' when I don't want them interfering with an assignment.	[reply]
Re^4: faulty expression prob by ikegami (Patriarch) on Apr 04, 2008 at 00:32 UTC
Re^2: faulty expression prob by apl (Monsignor) on Apr 03, 2008 at 10:59 UTC
Thanks for breaking out the assignments. I kept wondering if perl-diddler didn't actually want equality tests as opposed to assignments in his original code...	[reply]
Re: faulty expression prob by grizzley (Chaplain) on Apr 03, 2008 at 07:33 UTC
First what I've though was that the parens are causing the problem like in print: `$rh=1; $dist=1; print ($rh and $dist) ? 2 : -2` [download] But quick check `#!perl sub aa { $rh=shift; $dist=shift; return ($rh and $dist) ? 2 : -2; } print aa;` [download] has showed me I was wrong :) But I want to point at the issue - be very careful with paren at the beginning of parameters' list.	[reply] [d/l] [select]
Re^2: faulty expression prob by parv (Parson) on Apr 04, 2008 at 03:45 UTC
I have stopped assuming that `print (...) ...` would always do the right thing, so `print +( ... ) ...` is what I write when there is even a hint of doubt. `:/`	[reply] [d/l] [select]