[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Re: [SCM] Debian package checker branch, master, updated. 2.5.2-11-g39af0d7



On 2011-08-14 00:56, Russ Allbery wrote:
> Niels Thykier <niels@thykier.net> writes:
> 
>> Turns out this particular these two were not Perlcritic issues (I
>> applied these fixes from my memory of previous Perlcritic issues).
> 
> Oh!  Okay.  I'm actually surprised on the variable one, as I've seen
> things like that from perlcritic before.
> 

It is possible that the warning is there, but was disabled by our
perlcritic settings.  Raphael deployed a .perlcriticrc using a whitelist
(and one in t/scripts/critic.t).

>>   So the "my vs. our", I seem to have confused "our" with "use vars"  in
>> terms of Perlcritic warnings.  As for "my vs. our", it is my
>> understanding that "our" makes the variable "public"[0] and it seems to
>> me these variables are not actually intended to be used outside of
>> checks/standards-version.
> 
> It does, sort of, in the sense that some other module could access those
> variables from inside the namespace for that check, and my does prevent
> that.  So I suppose this is more of a style thing.  I always use our for
> global constants or things initialized at startup, combined with the
> all-caps naming convention.  I'm not sure if there's an official style, or
> what's the most common.
> 

I usually buy the "All caps" as global variables, but I still like to
differ between "exported/public" variables and "internal global" variables.
  I personally prefer the checks being completely self-contained and
would like to see shared constants (if any) moved to a separate module
that declares them as exported.  Partly because we still have code like:

"""
# If we have an unknown architecture, pretend that all [...]
if ($arch ne 'all' and not exists($ARCH_REGEX{$arch})) {
    debug_msg(1, "Unknown architecture: $arch");
    $ARCH_REGEX{$arch} = qr/./;
}
""" (checks/binaries)

My second reason is that I like to know that if I want to change (the
format of) a variable, I know I only have to check the current file and
nothing else.

A final note on it, is that we had variables in frontend/lintian
declared as "our" without being all caps... and one of them was actually
used in a check (the --checksums variable).
  If we do formalize a code-style, I hope we can agree that this is not
good. :)

> I suppose another alternative would be use constant, at least in some
> cases, although that creates a sub that then doesn't have a type prefix,
> and I think those look kind of strange.  But that may just be because I've
> been writing Perl for too long.  :)
> 

I normally like constants, but some how they feel "wrong" in perl...
like a second-grade citizen or so.  To use a constant in a regex you
have to do stuff like "${\CONSTANT}". >.>

>> On the other issue, I thought the "$C, @C"-thing was covered by the
>> "re-use" of same variable in lexical scope, but it turns out it isn't
>> (probably due to the "types" being different).
> 
> Ah.  Yes.  Each type of variable has its own separate namespace.
> 
>>   I can see the idea behind it now, sort of like a C-string where you
>> can process the string as a whole (strcmp(s, "text")) or individual
>> characters (s[0]).  I guess the perl way of doing that is to declare two
>> variables with same name and different type.
> 
> Yeah, or at least that's what I've always done in the past.
> 

All right, I am ready to buy that approach in moderate doses.  Maybe it
is a familiarity thing.  Perhaps we can declare both variables at the
same place.

>>   In the given case I can see the use of this.  That being said, I have
>> my concerns blessing wide-spread use of this.  You may remember I got a
>> bit confused with:
> 
>> """
>> $checks or ($checks = join(',',keys %check_info));
>> [...]
>> $checks{$c} = 1;
>> """
>>  from frontend/lintian in 2.4.3: $checks was a comma-separated list and
>> %checks was a hash where the keys were elements from said list.
> 
> Yeah.  I'm not sure if this is really confusing, or if it's just a
> familiarity thing.  I use the same name (but a different type) for any set
> of variables that all hold exactly the same contents but in a different
> data structure, and use a different variable name if the contents may be
> different.
> 

Coming from languages where it is not allowed, it is confusing[0].  :)
But when you have coded in Perl for a while it (also) becomes a
familiarity thing.

It broke one of my "assumptions"; namely a variable is only "declared at
most once in any given scope"[1].  So when I saw "my $checks;" in
frontend/lintian, it never crossed my mind to check for "my %checks;"
which appeared 10-12 lines further down (until Adam pointed it out over
IRC).

In hindsight, I had plenty of pointers to look for a declaration of
%checks (a hashref would have used ->{} rather than {} and strict
forbids/croaks on use of non-hashref as hashref, etc.), which is where
the familiarity part kicks in[2].
  For newcomers, I think we can save them a bit of confusion by at least
comment when we do reuse the variable name in these cases.

[0] Even perldata agrees with me: XD

""" This means that $foo and @foo are two different variables. It also
means that $foo[1] is a part of @foo, not a part of $foo.  This may seem
a bit weird, but that's okay, because it is weird. """


[1] it may shadow an existing variable like, but there is only one of
them available via the "name" in the scope.

int a;
{
	int a; //shadows the original a
}

[2] Of course, back then I was close to declaring myself for stark,
raving mad because the variable acted like a hash and a scalar at the
same time.

>> Can you point me to the section on "my ($v, @v, %v);" in the perl
>> documentation (not really sure what to look/google for here).  I would
>> like to get a second look on this. :)
> 
> perldata provides some basic explanation of the different data types, the
> namespace separation, and how the data type prefixes work, near the top of
> the documentation page.
> 

Thanks, guess I should have read the fine print a bit better.  :)

~Niels


Reply to: