[PATCH] Correct/completes Overloading in XS mods
John Peacock
overloading from XS code. Unfortunately, I made a mistake and the current code
will cause problems (at least under multithreaded Perl). This patch fixes that
as well as completing the FALLBACK support (and adds docs to perlxs.pod).
I am going to try to craft a test which will exercise these features, but I need
to figure out how to sucessfully use MakeMaker with an uninstalled Perl first (I
know, something to do with building in ext/).
John
--
John Peacock
Director of Information Research and Technology
Rowman & Littlefield Publishing Group
4720 Boston Way
Lanham, MD 20706
301-459-3366 x.5010
fax 301-429-5747
Benjamin Goldberg
[snip]
> + TrimWhitespace($_) ;
> +
> + # check for TRUE/FALSE/UNDEF
> + death ("Error: FALLBACK: TRUE/FALSE/UNDEF")
> + unless /^(TRUE|FALSE|UNDEF)/i ;
Umm, so if I type in:
FALLBACK: true is false
it won't complain? Methinks you need to anchor the end, too.
Also, I would accept "0" and "1", too, as an undocumented feature.
[snip]
> +if ($Overload) # make it findable with fetchmethod
> +{
> +
> + print Q<<"EOF";
> +#XS(XS_${Packid}_nil); /* prototype to pass -Wmissing-prototypes */
> +#XS(XS_${Packid}_nil)
> +#{
> +# XSRETURN_EMPTY;
> +#}
> +#
> +EOF
> + unshift(@InitFileCode,
> + " newXS(\"$Package\::()\", XS_${Packid}_nil, file$proto);\n");
Putting in a sub named "Package::()" is done for the purpose of making
overload::Overloaded("Package") return true, and to make the package
findable via fetchmethod. IMO, you really and truly ought to include
such a comment with this line... eg:
unshift(@InitFileCode, <<"MAKE_FETCHMETHOD_WORK");
/* Making a sub named "Package::()" allows the package */
/* to be findable via fetchmethod(), and causes */
/* overload::Overloaded("Package") to return true. */
newXS("${Package}::()", XS_${Packid}_nil, file$proto);
MAKE_FETCHMETHOD_WORK
Note that if you change "XS_${Packid}_nil" to "pp_null", as you youself
suggested, then you don't need to create XS_${Packid}_nil. Assuming of
course that that works.
PS: If pp_null works, then XS_version_noop could be replaced with that.
[snip]
> -# /* create the package stash */
> +# /* register the overloading magic */
> # HV *hv = get_hv(\"$Package\::OVERLOAD\",TRUE);
> # SV *sv = *hv_fetch(hv,"register",8,1);
> # sv_inc(sv);
> # SvSETMAGIC(sv);
All packages %{"${Package}::OVERLOAD"} hashes are automatically magical,
so that accessing them, or any of their elements, results in the global
variable PL_amagic_generation getting incremented. We can skip the hash
access and increment the global directly. So, replace the above 5 lines
with the following 2 lines:
# /* register the overloading (type 'A') magic */
# PL_amagic_generation++;
> -# /* Make it findable via fetchmethod */
> -# newXS(\"$Package\::()\", NULL, file);
Although this had been *partly* wrong in the first place (you need a
*valid* xsub, such as XS_${Packid}_nil, or possibly pp_null, not NULL),
you *do* still need to create a *sub* named "Package::()" for this
package's overloading to work.
> +# /* set the fallback handling */
> +# sv = get_sv( \"${Package}\::()\", TRUE);
> +# sv_setsv( sv, ${Fallback} );
Merely making a *scalar* named "Package::()" isn't sufficient to make
the overloading findable, I don't think. In other words, you need both.
I think.
So...
# /* Make *{"${Package}::()"} findable via gv_fetchmeth() */
# newXS("${Package}::()", pp_null, file);
# /* The magic for overload gets a GV* via gv_fetchmeth as */
# /* mentioned above, and looks in the SV* slot of it for */
# /* the "fallback" status. */
# sv_setsv(
# get_sv( "${Package}::()", TRUE ),
# $Fallback
# );
PS: If there *was* no FALLBACK keyword, I would prefer to not even have
this sv_setsv bit here... there's really no need to set it in such a
case, since the default is undef.
> # }
> EOF
>
> --- perl-current.test/pod/perlxs.pod.orig Fri Jun 7 10:33:07 2002
> +++ perl-current.test/pod/perlxs.pod Sat Aug 31 08:44:32 2002
> @@ -1260,6 +1260,23 @@ characters, you must type the parameter
> multiple overloads with whitespace. Note that "" (the stringify
> overload) should be entered as \"\" (i.e. escaped).
>
> +=head2 The FALLBACK: Keyword
> +
> +In addition to the OVERLOAD keyword, if you need to control how
> +Perl autogenerates missing overloaded operators, you can set the
> +FALLBACK keyword, like this:
> +
> + void
> + rpcb_gettime_noproto()
> + FALLBACK: TRUE
> + ...
FALLBACK shouldn't need to be associated with any subroutine. According
to this example, it does.
> +where FALLBACK can take any of the three values TRUE, FALSE, or
> +UNDEF. If you do not set any FALLBACK value when using OVERLOAD,
> +it defaults to UNDEF. FALLBACK is not used except when one or
> +more functions using OVERLOAD have been defined. Please see
> +L<overload/Fallback> for more details.
> +
> =head2 The INTERFACE: Keyword
>
> This keyword declares the current XSUB as a keeper of the given
--
tr/`4/ /d, print "@{[map --$| ? ucfirst lc : lc, split]},\n" for
pack 'u', pack 'H*', 'ab5cf4021bafd28972030972b00a218eb9720000';
John Peacock
>
> Umm, so if I type in:
> FALLBACK: true is false
> it won't complain? Methinks you need to anchor the end, too.
>
> Also, I would accept "0" and "1", too, as an undocumented feature.
>
Good points all. Consider it done...
> [snip]
>
> Putting in a sub named "Package::()" is done for the purpose of making
> overload::Overloaded("Package") return true, and to make the package
> findable via fetchmethod. IMO, you really and truly ought to include
> such a comment with this line... eg:
>
> unshift(@InitFileCode, <<"MAKE_FETCHMETHOD_WORK");
> /* Making a sub named "Package::()" allows the package */
> /* to be findable via fetchmethod(), and causes */
> /* overload::Overloaded("Package") to return true. */
> newXS("${Package}::()", XS_${Packid}_nil, file$proto);
> MAKE_FETCHMETHOD_WORK
Also very a good suggestion (I didn't stop to think that I could stuff comments
into @InitFileCode, too). Ditto.
>
> Note that if you change "XS_${Packid}_nil" to "pp_null", as you youself
> suggested, then you don't need to create XS_${Packid}_nil. Assuming of
> course that that works.
>
> PS: If pp_null works, then XS_version_noop could be replaced with that.
I cannot see how to make pp_null work in this case. pp_null is defined as
PP(pp_null), where that macro is this:
#define PP(s) OP * Perl_##s(pTHX)
and I need something defined as XS(package_nil), where that macro is this:
#define XS(name) void name(pTHX_ CV* cv)
I couldn't figure out a way which would make pp_null look like an xsub. I could
certainly see creating a xs_null with the same behavior in XSUB.h, so that all
XS modules could share it.
>
>
> All packages %{"${Package}::OVERLOAD"} hashes are automatically magical,
> so that accessing them, or any of their elements, results in the global
> variable PL_amagic_generation getting incremented. We can skip the hash
> access and increment the global directly. So, replace the above 5 lines
> with the following 2 lines:
>
> # /* register the overloading (type 'A') magic */
> # PL_amagic_generation++;
Neat, but how does that associate it with this object type? Those five lines
were a direct translation of the associated Perl code in overload.pm. If I was
using cargo cult logic, I'm more than happy to change it.
>
>
>>-# /* Make it findable via fetchmethod */
>>-# newXS(\"$Package\::()\", NULL, file);
>
>
> Although this had been *partly* wrong in the first place (you need a
> *valid* xsub, such as XS_${Packid}_nil, or possibly pp_null, not NULL),
> you *do* still need to create a *sub* named "Package::()" for this
> package's overloading to work.
That's why I did this patch; I know now that I need a valid xsub, so I created
it above. Then I associated with the correct xsub below. I'm confused about
this complaint.
>
>
>>+# /* set the fallback handling */
>>+# sv = get_sv( \"${Package}\::()\", TRUE);
>>+# sv_setsv( sv, ${Fallback} );
>
>
> Merely making a *scalar* named "Package::()" isn't sufficient to make
> the overloading findable, I don't think. In other words, you need both.
> I think.
That's why I did both. These are the two lines in overload.pm
*{$package . "::()"} = \&nil; # Make it findable via fetchmethod.
${$package . "::()"} = $fb; # Make it findable too (fallback only).
The first looks like setting the glob and the second certainly seems to be to be
a scalar in that namespace. So, I need the newXS() line as well as the scalar
assignment, don't I? If the first line is strictly setting the CV on the GV and
the second is the SV on the same GV, fine.
>
> So...
> # /* Make *{"${Package}::()"} findable via gv_fetchmeth() */
> # newXS("${Package}::()", pp_null, file);
> # /* The magic for overload gets a GV* via gv_fetchmeth as */
> # /* mentioned above, and looks in the SV* slot of it for */
> # /* the "fallback" status. */
> # sv_setsv(
> # get_sv( "${Package}::()", TRUE ),
> # $Fallback
> # );
>
> PS: If there *was* no FALLBACK keyword, I would prefer to not even have
> this sv_setsv bit here... there's really no need to set it in such a
> case, since the default is undef.
I thought about that, but then the code to embed this into xsubpp would need to
branch based on the value of $Fallback, which would make it more complex. I
didn't think a single scalar in a package namespace _we_ just created would be
much overhead.
>
> FALLBACK shouldn't need to be associated with any subroutine. According
> to this example, it does.
>
My mistake; when testing I was putting FALLBACK in the MODULE definition part of
the xs file. I'll change that too.
See attached revised patch based on Benjamin's suggestions.
John Peacock
> Right, nevermind ... I was confused myself due to the fact that you were
> moving the newXS("...()",...) from one part of xsubpp to another.
And I was only doing that because I was originally protecting the stash creation
in a context {} and I wanted all of the newXS() functions to be together. I
now don't need to do that at all, so I could move that push(@Init...) stuff into
the later block and just create the xsub itself up higher.
> I was looking through the source of Gv_AMupdate, and ...
>
> You only need the sub named "()" for it to be findable. The scalar
> named "()" is not to make the overloading findable, it's *only* to
> determine whether fallback is true/false/undef. The only reason that
> fallback-ness is in "()", not in, say, "(fallback", as one might expect,
> is because it's faster to do one fetch than two.
So the comments in overload.pm are misleading. Big suprise there!
>
> However, Gv_AMupdate fetches the SV from the GV, and uses it (tests for
> SvTRUE() on it, and SvOK() on it) without testing for whether or not the
> sv is NULL. Although SvTRUE tests for NULL-ness, SvOK does not.
You want (me) to provide a prophylatic patch to Gv_AMupdate to fix that?
>
> I couldn't provoke a segfault via "perl -e ...", but that might be
> because I'm not trying hard enough.
>
> perl -le '*{"()"}=sub{};${"OVERLOAD"}{x}++;print bless[]'
> [doesn't segfault ... I'm not sure why not.]
>
Perl is actually smarter than the two of us put together? :~)
Benjamin Goldberg
>
> Benjamin Goldberg wrote:
[snip]
> > All packages %{"${Package}::OVERLOAD"} hashes are automatically
> > magical, so that accessing them, or any of their elements, results
> > in the global variable PL_amagic_generation getting incremented. We
> > can skip the hash access and increment the global directly. So,
> > replace the above 5 lines with the following 2 lines:
> >
> > # /* register the overloading (type 'A') magic */
> > # PL_amagic_generation++;
>
> Neat, but how does that associate it with this object type?
If I understand the code correctly, the next time that an object is
blessed into the class, Gv_AMG will call Gv_AMupdate on that stash.
> Those five lines were a direct translation of the associated Perl code
> in overload.pm. If I was using cargo cult logic, I'm more than happy
> to change it.
>
> >
> >
> >>-# /* Make it findable via fetchmethod */
> >>-# newXS(\"$Package\::()\", NULL, file);
> >
> >
> > Although this had been *partly* wrong in the first place (you need a
> > *valid* xsub, such as XS_${Packid}_nil, or possibly pp_null, not
> > NULL), you *do* still need to create a *sub* named "Package::()" for
> > this package's overloading to work.
>
> That's why I did this patch; I know now that I need a valid xsub, so I
> created it above. Then I associated with the correct xsub below. I'm
> confused about this complaint.
Right, nevermind ... I was confused myself due to the fact that you were
moving the newXS("...()",...) from one part of xsubpp to another.
> >>+# /* set the fallback handling */
> >>+# sv = get_sv( \"${Package}\::()\", TRUE);
> >>+# sv_setsv( sv, ${Fallback} );
> >
> > Merely making a *scalar* named "Package::()" isn't sufficient to
> > make the overloading findable, I don't think. In other words, you
> > need both.
> > I think.
>
> That's why I did both. These are the two lines in overload.pm
>
> *{$package . "::()"} = \&nil; # Make it findable via fetchmethod.
> ${$package . "::()"} = $fb; # Make it findable too (fallback only).
>
> The first looks like setting the glob and the second certainly seems
> to be to be a scalar in that namespace. So, I need the newXS() line
> as well as the scalar assignment, don't I? If the first line is
> strictly setting the CV on the GV and the second is the SV on the same
> GV, fine.
I was looking through the source of Gv_AMupdate, and ...
You only need the sub named "()" for it to be findable. The scalar
named "()" is not to make the overloading findable, it's *only* to
determine whether fallback is true/false/undef. The only reason that
fallback-ness is in "()", not in, say, "(fallback", as one might expect,
is because it's faster to do one fetch than two.
However, Gv_AMupdate fetches the SV from the GV, and uses it (tests for
SvTRUE() on it, and SvOK() on it) without testing for whether or not the
sv is NULL. Although SvTRUE tests for NULL-ness, SvOK does not.
I couldn't provoke a segfault via "perl -e ...", but that might be
because I'm not trying hard enough.
perl -le '*{"()"}=sub{};${"OVERLOAD"}{x}++;print bless[]'
[doesn't segfault ... I'm not sure why not.]
> > So...
> > # /* Make *{"${Package}::()"} findable via gv_fetchmeth() */
> > # newXS("${Package}::()", pp_null, file);
> > # /* The magic for overload gets a GV* via gv_fetchmeth as */
> > # /* mentioned above, and looks in the SV* slot of it for */
> > # /* the "fallback" status. */
> > # sv_setsv(
> > # get_sv( "${Package}::()", TRUE ),
> > # $Fallback
> > # );
> >
> > PS: If there *was* no FALLBACK keyword, I would prefer to not even
> > have this sv_setsv bit here... there's really no need to set it in
> > such a case, since the default is undef.
>
> I thought about that, but then the code to embed this into xsubpp
> would need to branch based on the value of $Fallback, which would make
> it more complex. I didn't think a single scalar in a package
> namespace _we_ just created would be much overhead.
Not to mention that the source of Gv_AMupdate *seems* to indicate that
this *should* produces a segfault.
Tels
>Benjamin Goldberg wrote:
>>
>> Umm, so if I type in:
>> FALLBACK: true is false
>>
>> Also, I would accept "0" and "1", too, as an undocumented feature.
>>
>
>Good points all. Consider it done...
"Meeeeeeeeep! Bzzzt! Meeeeeeeeep! Undocumented feature alert! Meeeeeeeeep!
Please stop coding, bring your coding chairs into an upright position and
start thinking your problem over again."
:)
Seriously, why is it good idea to add an undocumented feature? Nobody knows
about this, so nobody can/will use it, except the unfortunate soul who sees
his/her program break when the undoc. feature is pulled back out again in
the near future by someone. Or someone needs to add the doc later. And in
any case quite a lot of time might be wasted in discussions whether the
undoc. feature is an accident, intent, or was just forgotten in the doc and
testcases.
So, please do something about it. :)
+sub FALLBACK_handler()
+{
+ # the rest of the current line should contain either TRUE,
+ # FALSE or UNDEF
+
+ TrimWhitespace($_) ;
+
+ # check for TRUE/FALSE/UNDEF
+ death ("Error: FALLBACK: TRUE/FALSE/UNDEF")
+ unless /^(TRUE|1|FALSE|0|UNDEF)$/i ;
The code and the comment are already out of sync. I would strongly suggest
to *not* do it this way.
+ $Fallback = "PL_sv_yes" if $1 =~ /TRUE|1/ ;
+ $Fallback = "PL_sv_no" if $1 =~ /FALSE|0/ ;
+ $Fallback = "PL_sv_undef" if $1 eq 'UNDEF' ;
If you die above with /i; and then test below without /i, then a
"true" instead of a "TRUE" will cause trouble (e.g. neglet to set Fallback
at all). I guess the /i after death() should be removed.
It is probably a good tme to write a testsuite for this and think about all
possible and impossible cases, and *then* start coding the features so that
all tests pass :)
Just one more nit:
if ($1 =~ /TRUE|1/)
{
$Fallback = "PL_sv_yes";
}
elsif ($1 =~ /FALSE|0/)
{
$Fallback = "PL_sv_no";
}
else
{
$Fallback = "PL_sv_undef";
}
Might be a bit faster (only one compare for TRUE, and two for FALSE and
UNDEF instead of three for each case). If you skip 0|1, you can make them
"eq" which should also be a bit faster than =~.
'tels_foo' is the TRUE|1 variant above, 'eq_foo' the one with " eq 'TRUE':
Benchmark: running john_false, john_true, john_undef, tels_false,
tels_true, tels_undef for at least 3 CPU seconds...
john_false: 3s ( 3.09 usr + 0.01 sys = 3.10 CPU) @ 228816/s (n=709330)
john_true: 3s ( 3.13 usr + 0.00 sys = 3.13 CPU) @ 248428/s (n=777581)
john_undef: 3s ( 3.12 usr + 0.00 sys = 3.12 CPU) @ 173683/s (n=541894)
tels_false: 3s ( 3.08 usr + 0.00 sys = 3.08 CPU) @ 235633/s (n=725750)
tels_true: 3s ( 3.17 usr + 0.01 sys = 3.18 CPU) @ 507418/s (n=1613592)
tels_undef: 2s ( 3.13 usr + 0.00 sys = 3.13 CPU) @ 174879/s (n=547374)
eq_false: 2s ( 3.10 usr + 0.00 sys = 3.10 CPU) @ 728358/s (n=2257910)
eq_true: 3s ( 3.08 usr + -0.01 sys = 3.07 CPU) @ 855828/s (n=2627395)
eq_undef: 2s ( 3.14 usr + 0.01 sys = 3.15 CPU) @ 716799/s (n=2257919)
Of course, this is just an academically example and not intended as real
feedback since that particular codepath is probably not used often enough
to go to the trouble for an optimization :)
Hope this helps,
Tels
- --
perl -MDev::Bollocks -e'print Dev::Bollocks->rand(),"\n"'
preemptively initiate fourth-generation markets
http://bloodgate.com/perl My current Perl projects
PGP key available on http://bloodgate.com/tels.asc or via email
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.0.6 (GNU/Linux)
Comment: When cryptography is outlawed, bayl bhgynjf jvyy unir cevinpl.
iQEVAwUBPXJ6ZXcLPEOTuEwVAQFdrwf+NHBxdSt8DVnfOy0I10MVBY93A/6uRhqM
l3r3R3OqmiXgGez9b2eka0D+7f1tk4PX991y3LkjmbdzDeCxxdNOqC5/8A7TGWDP
Cj53ShFff/hbNeX5Av0Q8RLRa/uMGmqBQLrJpcVMQNKv+IOmVXjuIvm4mJ1GOr8Y
+ru0JPWYmV2PN5jocI4KkRRdxjd5I0WK/opAa51uLDMTcaBI5MHFeur85X+7f0Ki
HkitvxWO5tPqcaPcM08cAAfjTuWnfNjLf4yuKsrk6eOmct3XXWNq1UVf/RGhCtR6
fN40dzE6ePDE/jWnbsgwCgqZ20fEMvxZCNA/uSMP0j3e2HTvtXU9KQ==
=zgsX
-----END PGP SIGNATURE-----
Benjamin Goldberg
>
> Benjamin Goldberg wrote:
[snip]
> > for SvTRUE() on it, and SvOK() on it) without testing for whether or
> > not the sv is NULL. Although SvTRUE tests for NULL-ness, SvOK does
> > not.
>
> You want (me) to provide a prophylatic patch to Gv_AMupdate to fix
> that?
Not necessarily -- it's quite possible that I'm reading the code
incorrectly -- what I'd like you to do is apply your own eyes to
Gv_AMupdate, and see if I'm not misreading it. If a seg fault really is
possible, *then* it needs a patch.
> > I couldn't provoke a segfault via "perl -e ...", but that might be
> > because I'm not trying hard enough.
> >
> > perl -le '*{"()"}=sub{};${"OVERLOAD"}{x}++;print bless[]'
> > [doesn't segfault ... I'm not sure why not.]
>
> Perl is actually smarter than the two of us put together? :~)
Indeed -- if so, then the code is already right :)
Any idea why it doesn't segfault when the code looks like it should?
Rafael Garcia-Suarez
> > > perl -le '*{"()"}=sub{};${"OVERLOAD"}{x}++;print bless[]'
> > > [doesn't segfault ... I'm not sure why not.]
> >
> > Perl is actually smarter than the two of us put together? :~)
>
> Indeed -- if so, then the code is already right :)
>
> Any idea why it doesn't segfault when the code looks like it should?
That's the kind of sentence that rings a valgrind bell in my brain :
$ valgrind ./perl -le '*{"()"}=sub{};${"OVERLOAD"}{x}++;print bless[]'
==15011== valgrind-1.0.0, a memory error detector for x86 GNU/Linux.
==15011== Copyright (C) 2000-2002, and GNU GPL'd, by Julian Seward.
==15011== Estimated CPU clock rate is 804 MHz
==15011== For more details, rerun with: -v
==15011==
main=ARRAY(0x41208cec)
==15011==
==15011== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
==15011== malloc/free: in use at exit: 52666 bytes in 690 blocks.
==15011== malloc/free: 812 allocs, 122 frees, 59518 bytes allocated.
==15011== For a detailed leak analysis, rerun with: --leak-check=yes
==15011== For counts of detected errors, rerun with: -v
Sorry I'm not more helpful...
Nicholas Clark
> I am going to try to craft a test which will exercise these features, but I
> need to figure out how to sucessfully use MakeMaker with an uninstalled
> Perl first (I know, something to do with building in ext/).
I know that the test for ExtUtils::Constant builds extensions against an
uninstalled perl, and even runs a Makefile.PL (IIRC) against an
uninstalled perl. I would expect that the some of other tests in ExtUtils
also do things with the MakeMaker of the uninstalled perl, and may be better
to look at first.
Nicholas Clark
--
Even better than the real thing: http://nms-cgi.sourceforge.net/
h...@crypt.org
:John Peacock wrote:
:> Benjamin Goldberg wrote:
:> > because I'm not trying hard enough.
:> >
:> > perl -le '*{"()"}=sub{};${"OVERLOAD"}{x}++;print bless[]'
:> > [doesn't segfault ... I'm not sure why not.]
:>
:> Perl is actually smarter than the two of us put together? :~)
:
:Indeed -- if so, then the code is already right :)
:
:Any idea why it doesn't segfault when the code looks like it should?
IIRC when we create a glob we always stick sv_undef into the SV slot.
If so, that'd be enough to explain this.
Hugo
Nick Ing-Simmons
Actually we always create GvSV.
>If so, that'd be enough to explain this.
>
>Hugo
--
Nick Ing-Simmons
http://www.ni-s.u-net.com/
h...@crypt.org
:See attached revised patch based on Benjamin's suggestions.
Thanks, applied as #17832.
The case-(in)sensitive mismatch still existed, but I didn't like the
rematching of the text anyway so I fiddled with it some, as below.
Hugo
---
my %map = (
TRUE => "PL_sv_yes", 1 => "PL_sv_yes",
FALSE => "PL_sv_no", 0 => "PL_sv_no",
UNDEF => "PL_sv_undef",
) ;
# check for valid FALLBACK value
death ("Error: FALLBACK: TRUE/FALSE/UNDEF") unless exists $map{uc $_} ;
$Fallback = $map{uc $_} ;
John Peacock
> John Peacock <jpea...@rowman.com> wrote:
> :See attached revised patch based on Benjamin's suggestions.
>
> Thanks, applied as #17832.
>
> The case-(in)sensitive mismatch still existed, but I didn't like the
> rematching of the text anyway so I fiddled with it some, as below.
>
Thanks. That _is_ better code...