in reply to ExtUtils::ParseXS has changed

Well, you certainly should not put non-arguments there. The line after "pdl* x" should declare the type of the "position" argument. It should not be some random bit of code about "pos", which is not one of the arguments.

The fact that it used to work is certainly an accident.

But the correct thing to do is not what you proposed but more like: 

SV *
at_c(x,position)
        pdl*    x
        SV*     position
    PREINIT:
        PDL_Long * pos;
    CODE:
        ...
    OUTPUT:
        RETVAL

[download]

Then you can also replace "ST(1)" with "position" in the code.

"The PREINIT: keyword allows extra variables to be declared immediately before or after the declarations of the parameters from the INPUT: section are emitted."

- tye        

Replies are listed 'Best First'.
Re^2: ExtUtils::ParseXS has changed (PREINIT:)
by syphilis (Archbishop) on Aug 18, 2011 at 01:02 UTC
    The fact that it used to work is certainly an accident

    Ok - that makes good sense to me, and is one of the things I was hoping to establish.

    Then you can also replace "ST(1)" with "position" in the code.

    Some of us were wondering what this unused "position" argument was doing. Obviously it wasn't doing anything, but I think I start to understand a purpose for it. (Update: Duh !! It's just an arg that can now be accessed by its name instead of its position on the stack.)

    As I now understand it, it's *all* of the other variables (not just 'pos') that should be declared in PREINIT instead of CODE.

    I'll rewrite the file in accordance with tye's advice and give it a run.
    Many thanks to all respondents.

    Cheers,
    Rob
[reply]
      Being the guy who did the changes to ExtUtils::ParseXS, let me add my support for what tye said: The code in question worked purely by accident. The proper way to declare or initialize variables in an XSUB is in a PRE_INIT block! ExtUtils::ParseXS 3.XX just became strict in telling you about the problem in your code. It could have instead broken your code to segfault because of the old behaviour being an accidental misfeature and side-effect of the horrors that lurk in EU::ParseXS' code generation.
[reply]
        let me add my support for what tye said

        Yep - the file in question has now been amended in line with tye's recommendation, and all is well.
        Interestingly enough, that file had been in that state since 1998, and had produced no ill effects (afawk). There was even one abomination that did essentially this: 
        void
foo(x,dims)
    pdl *x
    PDL_Long *dims = NO_INIT
    CODE:
          dims = bar(ST(1));
          ...

        [download]
        'dims' (the 2nd argument supplied to foo) is an SV* ... but then there's a second 'dims' which is a PDL_Long*.
        Dunno how that worked ...

        ExtUtils::ParseXS 3.XX just became strict in telling you about the problem in your code

        Which is a good thing - though it didn't exactly complain in the right way.
        It merely told us that there was no typemap entry for PDL_Long*. And, sure enough, inserting a typemap entry for PDL_Long* fixed the problem, though that's the wrong fix.

        Cheers,
        Rob
[reply]
[d/l]

      As I now understand it, it's *all* of the other variables (not just 'pos') that should be declared in PREINIT instead of CODE.

      C doesn't allow intermixing of vars declarations and code like Perl and C++. Var declarations can just be placed globally or at the start of the block. PREINIT will be placed before any code. CODE can be preceded by other code. Therefore, declarations must be placed in PREINIT or you must add curlies to CODE.

      The PREINIT section of perlxs explains this in more detail and gives example.

[reply]
[d/l]
[select]
        C99 does in fact allow declarations anywhere.
[reply]

In Section Seekers of Perl Wisdom