phlummox / hs-perl5 Goto Github PK

The withPerl5 function has a rather opaque signature that gives no clue that it's effectively "bracket"-ing an implicit, global Interpreter.

The function's usage could be made more obvious if Interpreter objects were explicitly allocated and finalized (rather than a single global instance being operated on). Then, withPerl5 would take a function which took an Interpreter object, its signature would give a better idea of its purpose, and it'd be more flexible, too.

This shouldn't be hard to do - the perlembed docco gives examples of how to work with multiple interpreter instances.

Some within-living-memory efforts to embed Perl in a C program on Windows can be seen at:

• StackOverflow - Attempt to embed Perl in C on Windows
• Perlmonks - Re: embedding Perl into C for Windows

As I don't have easy access to a Windows machine, I may have to set up a VM and hope for the best. chocolatey provides both activeperl and strawberryperl packages which might work.

Programs using functions within withPerl5 sporadically segfault (or suffer other memory corruption -- sometimes glib will report errors before the segfault, like "corrupted double-linked list") when compiled with -threaded.

This is (almost certainly) because Perl is not thread-safe - it sets up various global data structures, and if multiple threads can access these at the same time, corruption is eventually inevitable.

The package should have been written so that all calls within withPerl5 occur on a single OS thread, so as to avoid this.

set up CI using Travis or GitHub Actions. Would be nice to also see if it builds OK on Windows, using Appveyor.

We use funcs like perl_sv_2pvutf8 which don't exist in Perl 5.6.x - so ensure the documentation states the correct minimum version.

If C constants like G_VOID, G_SCALAR are out of sync with our Haskell code, that can lead to memory corruption. Values for the "call.." flags were updated in 1ff0054, but we should extract them directly from the header file to ensure they don't get out of sync again.

The C function SV ** hsperl_return_conv (int count) has a needlessly complex way of returning values and errors – it allocates an array of SVs, with up to two NULL terminators (explanation here),

It'd be better to just provide an "out" parameter, that might contain return values or errors, and use an int return value to indicate which is the case. But also - there doesn't seem to be any reason to provide multiple error values. (I think one is intended to be always a string, the other, potentially, a reference to a more complex Perl structure.) We can just provide one, and let the caller work out how best to deal with it.

For instance, for hsperl_newSViv, the C prototype given is:

hsperl_newSViv ( int iv )

-- limiting us to passing a C int, when the actual parameter type is an "IV" (usually a 64-bit int). There is no reason for this, we can accurately represent the Haskell equivalent of Perl types like IV and NV using hsc2hs. And by using correct types, we can write accurate tests around the boundaries of those types (e.g. checking that maxBound of an Int64 converts correctly).

The Cabal API has changed -- Setup.hs needs to be amended to handle this.

The hsperl_init C function is poorly behaved in the event it fails to parse a given Perl-code "preamble": (a) it prints to stderr, bad behaviour for a library, and (b) calls exit(), terminating the whole process. See here:

    if (SvTRUE(ERRSV)) {
        STRLEN n_a;
        // TODO: see if we can raise a proper exception instead
        fprintf(stderr, "Error init perl: %s\n", SvPV(ERRSV,n_a));
        // NO. Should _NOT_ be aborting, mid-program.
        exit(1);
    }

This should be fixed – we should indicate errors through our return value. Either we can return a NULL pointer on error, and provide an "out" parameter where an error code can be set; or we can have an int return value (0 for success, anything else for failure) and return the Interpreter pointer via an Interpreter **.

The particular condition above is admittedly unlikely to occur – unless the preamble being parsed is deliberately changed, Perl should always be able to parse it – so this is low-ish priority, but no doubt there are other errors in hsperl_init we're not currently checking for.

The String instances for ToSV and FromSV seem flakey - they only reliable work with ASCII-only strings containing no NUL characters. (IIRC, they use withCStringLen, and I thought that could handle NULs, but perhaps not.)

In any case, Text is a far better string type to use.

perl5_eval and perl5_apply require the caller to free the returned memory -- but currently they don't, leading to memory leaks.

perl_configure.sh is currently just some perl invocations via the shell - use process instead for a pure Haskell implementation. Also improve on the hard-coded "extra libraries" list it uses.

Given that Perl 5 is the only Perl that is still in use (Perl 6 is now "Raku"), the inconsistent presence of "..5" in module and function names can probably be dropped. (Anyone who somehow manages to get a conflict between this package, and some embedding of Perl 4, the last version of which was released in 1993, would presumably know what they were getting themselves into.) At any rate, the "5" can definitely be dropped from function names.