Ramkumar Ramachandra <artagnon@xxxxxxxxx> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html