Developers of Sympa

[Guillaume Rousse <address@concealed>]

Le 25/02/2014 11:48, David Verdin a écrit :
> However, I think it is important to test the presence / absence of
> expected parameters just after the "my (%params) = @_;" and translate
> them into local variables. We could even undef the %params hash just
> after converting its values to local variables.
> That way, if we had a new key/value to the params hash and try to use it
> directly in the code a $hash{$key}, we woul find the b_ug at compile time.
Multiple issues here.

Testing the presence (and eventually the type) of expected parameters 
matters, indeed, but I'd restrict it to public functions/methods only. 
For private functions/methods, that are supposed to be called from the 
same package only, this is mostly useless, and should be assumed to be 
done by the caller.

# this function can be called by any part of sympa
sub function {
    my (%params) = @_;

    # check params first
    if (!$params{a}) {
        # handle error
    }

    # do something
}

# this function can be called only by current package
sub _function {
    my (%params) = @_;

    # assume parameters have been checked by caller

    # do something
}

Second, assuming than copying parameters into local variables always 
brings additional benefits from compilation-time checks is discussable, 
especially for short functions, ie:

sub function {
    my (%params) = @_;

    return $params{a} . '@' . $params{b};
}

sub function {
    my (%params) = @_;

    my $a = $params{a};
    my $b = $params{b};

    return $a . '@' . $b;
}

The potential typo issue in the actual code of the function in the 
second form is actually just displaced in the local variable 
affectation: no safety gain, with two additional variable copies.

Third, the ability to reuse initial parameters list directly is very 
useful in some circunstances, such as wrapping functions with other 
functions, ie:

sub generic_function {
    my (%params) = @_;
    ...
    return $something;
}

sub less_generic_function {
    return generic_function(@_, foo => 'value');
}

That's exactly the case of current wwslog() function, for instance, that 
just wraps do_log() function. I wouldn't call it a bug, and even less 
enforce a mandatory removal of initial parameters list.

Last, I think it is not reasonable to try to enforce a unique 
fit-them-all rule. Especially with regard to the current code base, the 
lack of workforce to enforce it, and the difficulties we have to find 
consensus, even on the smallest cosmetic issues.

The following example should adress the various concerns, and constitute 
a suitable *prefered* template for public functions/methods (ie: not the 
absolute rule):

sub function {
    my (%params) = @_;

    if (!$params{c}) {
         # log error and return
    }

    $a = defined $params{a} ? $params{a} : 'default for A';
    $b = defined $params{b} ? $params{b} : 'default for B';
    $c = $params{c);

    ...
}

Is that acceptable for everyone ?
--
Guillaume Rousse
INRIA, Direction des systèmes d'information
Domaine de Voluceau
Rocquencourt - BP 105
78153 Le Chesnay
Tel: 01 39 63 58 31

Attachment: smime.p7s
Description: Signature cryptographique S/MIME