Front page | perl.perl5.porters |
Postings from July 2007
Re: [PATCH] ExtUtils::ParseXS - Error reporting problem with INTERFACE and ALIAS keywords
Thread Previous
From:
Robert May
Date:
July 28, 2007 20:35
Subject:
Re: [PATCH] ExtUtils::ParseXS - Error reporting problem with INTERFACE and ALIAS keywords
Message ID:
54bdc7510707280823p1e7804d6q53ebc32dc2163d99@mail.gmail.com
On 20/07/07, Ken Williams <ken@mathforum.org> wrote:
> Hi Robert, hi p5p,
>
> I throw myself at the mercy of p5p on this one. Despite being the
> maintainer of EU::PXS I'm far from an expert on XS->C processing.
> P5p, any opinion on which alternative to apply? I like the cosmetic
> looks of #2 also.
Warnocked? In an attempt to make life easier for potential
reviewers, I'll ask the question a different way.
Currently ExtUtils::ParseXS (or xsubpp) generates 2 different
pre-ambles to XSUBS:
(1) Most cases:
if (items != 0)
Perl_croak(aTHX_ "Usage: %s(%s)", "XSUsage::one", "");
(2) When the ALIAS keyword is used:
if (items != 0)
Perl_croak(aTHX_ "Usage: %s(%s)", GvNAME(CvGV(cv)), "");
There are 2 problems with this:
(a) When using the INTERFACE keyword option (1) is generated, resulting in:
Perl_croak(aTHX_ "Usage: %s(%s)", "XSUsage::interface_v_i", "");
which reports errors from an unexpected sub name.
(b) options (1) and (2) are inconsistent in whether fully qualified
sub names are used:
Usage: XSUsage::one()
or Usage: one();
I have 2 proposals on the table (original mail with patches here:
http://www.xray.mpe.mpg.de/mailing-lists/perl5-porters/2007-07/msg00796.html).
My first proposal is a simple change to EU::PXS that uses option (2)
for subs generated using both the ALIAS *and* INTERFACE keywords.
This only resolves problem (a).
My second proposal is to replace the pre-amble in all cases with:
if (items != 0)
Perl_croak(aTHX_ "Usage: %s::%s(%s)", HvNAME(GvSTASH(CvGV(cv))),
GvNAME(CvGV(cv)), "");
I am happy that the first proposal won't break anything. I prefer the
second proposal, as it solves both problems, but can someone confirm
that thecode retrieves the package name correctly from the passed cv
in all circumstances; i.e. the cv arg passed into an XSUB can never
be NULL, or have other attributes that would cause this to fail:
char *package_name = HvNAME(GvSTASH(CvGV(cv)));
Regards,
Rob.
>
> -Ken
>
>
> On Jul 13, 2007, at 6:47 PM, Robert May wrote:
>
> > [ CC: Ken Williams <kwilliams@cpan.org>, listed maintainer of
> > ExtUtils::ParseXS ]
> >
> > I have a small issue with the following XSUB
> >
> > C:\WINDOWS\Desktop\test>type MyTest.xs
> > #include "EXTERN.h"
> > #include "perl.h"
> > #include "XSUB.h"
> >
> > int one() { return 1; }
> > int two() { return 2; }
> > int three() { return 3; }
> >
> > MODULE = MyTest PACKAGE = MyTest
> >
> > PROTOTYPES: DISABLE
> >
> > int
> > one()
> >
> > int
> > two()
> > ALIAS:
> > two_x = 1
> > FOO::two = 2
> >
> > int
> > interface_v_i()
> > INTERFACE:
> > three
> >
> > C:\WINDOWS\Desktop\test>type ntest.pl
> > #!perl -w
> > use strict;
> > use warnings;
> >
> > use MyTest;
> >
> > my @funcs = qw( MyTest::one
> > MyTest::two MyTest::two_x FOO::two
> > MyTest::three
> > );
> >
> > for my $func (@funcs) {
> > my $code = \&$func;
> > eval { $code->('somearg') };
> > warn $@ if $@;
> > }
> >
> > C:\WINDOWS\Desktop\test>perl -Mblib ntest.pl
> > Usage: MyTest::one() at ntest.pl line 14.
> > Usage: two() at ntest.pl line 14.
> > Usage: two_x() at ntest.pl line 14.
> > Usage: two() at ntest.pl line 14.
> > Usage: MyTest::interface_v_i() at ntest.pl line 14.
> >
> > Particularly the last line, which is generated by calling
> > MyTest::three()
> > with the wrong number of arguments. I think it is very confusing
> > for a
> > module user to be presented with an error message referring to a
> > sub they
> > didn't call (and is unlikely to be documented). This run also
> > highlights
> > that the code generated by xsubpp is inconsistent in whether it
> > uses fully
> > qualified sub names or not when reporting the error.
> >
> > I propose 2 (alternative) patches to ExtUtils::ParseXS (and ideally
> > to the
> > same area of xsubpp in maint). The full attached patches are
> > against the CPAN
> > release of ExtUtils::ParseXS (2.18), include tests, and update
> > MANIFEST.
> >
> > ALTERNATIVE 1:
> > --------------
> > This is the simplest change I could see, and fixes the error message
> > when using the INTERFACE keyword.
> >
> > diff -urN ExtUtils-ParseXS-orig\lib\ExtUtils\ParseXS.pm
> > ExtUtils-ParseXS\lib\ExtUtils\ParseXS.pm
> > --- ExtUtils-ParseXS-orig\lib\ExtUtils\ParseXS.pm Tue Jan 30
> > 02:56:14 2007
> > +++ ExtUtils-ParseXS\lib\ExtUtils\ParseXS.pm Fri Jul 13 19:43:24 2007
> > @@ -590,7 +590,7 @@
> > # *errbuf = '\0';
> > EOF
> >
> > - if ($ALIAS)
> > + if ($ALIAS || $INTERFACE)
> > { print Q(<<"EOF") if $cond }
> > # if ($cond)
> > # Perl_croak(aTHX_ "Usage: %s(%s)", GvNAME(CvGV(cv)),
> > "$report_args");
> >
> > C:\WINDOWS\Desktop\test>perl -Mblib ntest.pl
> > Usage: MyTest::one() at ntest.pl line 14.
> > Usage: two() at ntest.pl line 14.
> > Usage: two_x() at ntest.pl line 14.
> > Usage: two() at ntest.pl line 14.
> > Usage: three() at ntest.pl line 14.
> >
> > ALTERNATIVE 2:
> > --------------
> > This change makes the errors use fully qualified sub names when
> > using both
> > the INTERFACE and ALIAS keywords.
> >
> > This wants reviewing, as I am dabbling in areas of internals that I
> > don't
> > know well (at all?), but it attempts to *always* report using the
> > fully qualified
> > sub name (as typically happens when not using the INTERFACE or
> > ALIAS keyword).
> >
> > diff -urN ExtUtils-ParseXS-orig\lib\ExtUtils\ParseXS.pm
> > ExtUtils-ParseXS\lib\ExtUtils\ParseXS.pm
> > --- ExtUtils-ParseXS-orig\lib\ExtUtils\ParseXS.pm Tue Jan 30
> > 02:56:14 2007
> > +++ ExtUtils-ParseXS\lib\ExtUtils\ParseXS.pm Fri Jul 13 23:11:30 2007
> > @@ -590,20 +590,17 @@
> > # *errbuf = '\0';
> > EOF
> >
> > - if ($ALIAS)
> > - { print Q(<<"EOF") if $cond }
> > + if($cond)
> > + { print Q(<<"EOF"); }
> > # if ($cond)
> > -# Perl_croak(aTHX_ "Usage: %s(%s)", GvNAME(CvGV(cv)),
> > "$report_args");
> > +# Perl_croak(aTHX_ "Usage: %s::%s(%s)", HvNAME(GvSTASH(CvGV
> > (cv))),
> > +# GvNAME(CvGV(cv)),
> > +# "$report_args");
> > EOF
> > - else
> > - { print Q(<<"EOF") if $cond }
> > -# if ($cond)
> > -# Perl_croak(aTHX_ "Usage: %s(%s)", "$pname", "$report_args");
> > -EOF
> > -
> > + else {
> > # cv doesn't seem to be used, in most cases unless we go in
> > # the if of this else
> > - print Q(<<"EOF");
> > + print Q(<<"EOF"); }
> > # PERL_UNUSED_VAR(cv); /* -W */
> > EOF
> >
> > C:\WINDOWS\Desktop\test>perl -Mblib ntest.pl
> > Usage: MyTest::one() at ntest.pl line 14.
> > Usage: MyTest::two() at ntest.pl line 14.
> > Usage: MyTest::two_x() at ntest.pl line 14.
> > Usage: FOO::two() at ntest.pl line 14.
> > Usage: MyTest::three() at ntest.pl line 14.
> >
> > I'd welcome comments on which approach is better (I prefer
> > alternative 2),
> > and will happily try to wrap any suggestions into further patches.
> > This
> > is my first attempt at submitting a core patch, so if I've missed
> > anything
> > please let me know and I'll try to rectify things.
> >
> > Regards,
> > Rob.<patch.1><patch.2>
Thread Previous