Re: [PATCH 1/3] send-email: use "return;" not "return undef;" on error codepaths

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 2, 2013, 14:49 UTC
Message-ID: <7vsj39gf7k.fsf@alter.siamese.dyndns.org>
In-Reply-To: <CALkWK0muF2Nx=ykD6jt4pvpmp9c3R2KhXhQMjLAyMWiHu_6iPA@mail.gmail.com>

Ramkumar Ramachandra <artagnon@gmail.com> writes:

Show 13 quoted lines

> 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.