Re: [PATCH 1/3] send-email: use return; not return undef; on error codepaths
Junio C Hamano wrote: Note that we leave return undef; in validate_address on purpose, even though Perlcritic may complain. The primary return site of the function returns whatever is in the scaler variable $address, so it is pointless to change only the other return undef; to return. The caller must be prepared to see an array with a single undef as the return value from this subroutine anyway. The way I understood it: All callsites only ever call validate_address via validate_address_list, which in turn maps a list of addresses by calling validate_address on each address. Therefore, validate_address returning an undef in one codepath is correct. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] send-email: use return; not return undef; on error codepaths
Ramkumar Ramachandra artag...@gmail.com writes:
Junio C Hamano wrote:
Note that we leave return undef; in validate_address on purpose,
even though Perlcritic may complain. The primary return site of
the function returns whatever is in the scaler variable $address, so
it is pointless to change only the other return undef; to return.
The caller must be prepared to see an array with a single undef as
the return value from this subroutine anyway.
The way I understood it:
All callsites only ever call validate_address via
validate_address_list, which in turn maps a list of addresses by
calling validate_address on each address. Therefore, validate_address
returning an undef in one codepath is correct.
I think we are in agreement then.
The implication of what you just said is that anybody that calls
this subroutine _must_ be (and indeed is) prepared to see a single
'undef' returned from it, hence the use pattern,
@foo = validate_address(...);
if (@foo) { ... we got a valid one ... } else { ... we did not ... }
cannot be used for the subroutine _anyway_. @foo may get an invalid
address, even with the don't say 'return undef;' say 'return;'
wisdom is applied to the other return site in the subroutine.
Now, the subroutine's body essentially is:
while (!extract_valid($address)) {
...
if (...) {
return undef; # or just return;
}
}
return $address;
You can prove that the return $address on the last line will never
return 'undef' by proving that extract_valid() never returns true
for 'undef' input. With that, we can safely change the error return
to do return; in the loop.
When it is done, the subroutine's new interface becomes Single
address goes in, either a single valid address comes out and that
will never be an undef, or nothing (not even an undef) comes out
upon error.
Which means the sole caller of the subroutine needs to be updated,
which currently does this:
return grep { defined $_ } map { validate_address($_) } @_;
As the subroutine never returns an undef as no this is not valid,
but acts more like a filter to cull bad ones from the input, the
outer grep { defined $_ } becomes unnecessary.
And I think that change logically belongs to the same patch as the
one that inspects how validate_address behaves and updates its
sometimes we return undef to signal a bad input to return nothing.
That is why validate_adress was excluded from [1/3] which deals with
other simpler cases.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/3] send-email: use return; not return undef; on error codepaths
From: Ramkumar Ramachandra artag...@gmail.com
All the callers of ask, extract_valid_address, and validate_patch
subroutines assign the return values from them to a single scaler:
$var = subr(...);
and return undef; in these subroutine can safely be turned into a
simpler return;. Doing so will also future-proof a new caller that
mistakenly does this:
@foo = ask(...);
if (@foo) { ... we got an answer ... } else { ... we did not ... }
Note that we leave return undef; in validate_address on purpose,
even though Perlcritic may complain. The primary return site of
the function returns whatever is in the scaler variable $address, so
it is pointless to change only the other return undef; to return.
The caller must be prepared to see an array with a single undef as
the return value from this subroutine anyway.
Signed-off-by: Ramkumar Ramachandra artag...@gmail.com
Signed-off-by: Junio C Hamano gits...@pobox.com
---
git-send-email.perl | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index be809e5..79cc5be 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -711,7 +711,7 @@ sub ask {
}
}
}
- return undef;
+ return;
}
my %broken_encoding;
@@ -833,7 +833,7 @@ sub extract_valid_address {
# less robust/correct than the monster regexp in Email::Valid,
# but still does a 99% job, and one less dependency
return $1 if $address =~ /($local_part_regexp\@$domain_regexp)/;
- return undef;
+ return;
}
sub extract_valid_address_or_die {
@@ -1484,7 +1484,7 @@ sub validate_patch {
return $.: patch contains a line longer than 998
characters;
}
}
- return undef;
+ return;
}
sub file_has_nonascii {
--
1.8.2-441-g6e6f07b
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] send-email: use return; not return undef; on error codepaths
On Sun, Mar 31, 2013 at 9:40 PM, Junio C Hamano gits...@pobox.com wrote:
All the callers of ask, extract_valid_address, and validate_patch
subroutines assign the return values from them to a single scaler:
s/scaler/scalar/g
(note the /g)
$var = subr(...);
and return undef; in these subroutine can safely be turned into a
simpler return;. Doing so will also future-proof a new caller that
mistakenly does this:
@foo = ask(...);
if (@foo) { ... we got an answer ... } else { ... we did not ... }
Note that we leave return undef; in validate_address on purpose,
even though Perlcritic may complain. The primary return site of
the function returns whatever is in the scaler variable $address, so
it is pointless to change only the other return undef; to return.
The caller must be prepared to see an array with a single undef as
the return value from this subroutine anyway.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
