UCD.pm: Add check for parameter type

[khwilliamson]

This needs to be a scalar reference; I got burned with it not doing the right thing when it wasn't.

• I'm unsure if this set of changes requires a perldelta entry

[tonycoz]

This could use a test.

• I'm unsure if this set of changes requires a perldelta entry, and it is included.

I don't see the perldelta entry.

[jkeenan]

This could use a test.

Provided one at https://github.com/khwilliamson/perl5/pull/1

[tonycoz]

The test made obvious to me:

tony@venus:.../git/perl6$ ./perl -Ilib -MUnicode::UCD=num -e 'my $x; num("123", \$x)'
tony@venus:.../git/perl6$ ./perl -Ilib -MUnicode::UCD=num -e 'my $x=\1; num("123", \$x)'
Unicode::UCD::num: second parameter must be a scalar reference at -e line 1.

In both cases the parameter is a reference to $x, and $$x can be assigned to.

Another case would be:

tony@venus:.../git/perl6$ ./perl -Ilib -MUnicode::UCD=num -E 'my $x; num("123", bless \$x);'
Unicode::UCD::num: second parameter must be a scalar reference at -e line 1.

But I think that's more a case of "Doctor, it hurts when I do (something dumb)"

Alas builtin::reftype() also returns REF for the second case.

I'm not fond of this ref() behaviour (ref() -> REF describes the contents of the reference container, not the referenced container itself)

[jkeenan]

@tonycoz, what is your current opinion on this pull request? Can it be merged?

(Your May 19 comments suggest that in the course of reviewing this ticket, you came across larger problems with perl that are beyond the scope of this ticket. Is that impression correct?)

[tonycoz]

@tonycoz, what is your current opinion on this pull request? Can it be merged?

I've added more direct comments on the code. My comment on the handling of the ref() result was not addressed.

(Your May 19 comments suggest that in the course of reviewing this ticket, you came across larger problems with perl that are beyond the scope of this ticket. Is that impression correct?)

It's not a new problem.

ref() on a reference to a scalar can return REF or SCALAR, the first describes the content of the scalar, not the container as it always does for array and hash references.

I don't think it's fixable at this point.

[khwilliamson]

What if I loop, dereferencing each REF until I get to the base structure? Why doesn't that work?

[haarg]

I don't think recursing is correct. You don't want to modify a deep value, you just want to modify the direct thing referenced. But correctly checking for a scalar reference requires accepting more values from ref(): SCALAR, VSTRING, REF, GLOB, LVALUE, and REGEXP.

[tonycoz]

I suspect the best test is:

    croak __PACKAGE__, "::num: second parameter must be a scalar reference"
                        if defined $retlen_ref && !eval { $$retlen_ref = 0; 1 };

which directly tests exactly what we want, that $retlen_ref is a reference to a writable scalar, catching things like:

my $n1 = num($text, \1);
my $n2 = num($text, \$1);

Given the initialisation this does is repeated further down maybe they should be combined:

if (defined $retlen_ref) {
    eval { $$retlen_ref = 0; 1 }
        or croak __PACKAGE__, "::num: second parameter must be a scalar reference";
}
# eliminate the initialisation further down

explicit separate conditions to ensure the initialisation is obvious when reading the code.

[khwilliamson]

This is a much better solution than what I had done