#234 intrusive_ptr support for perl5/php
-*- text -*-
With these changes, (boost) intrusive_ptr support is now provided for
swig/perl and swig/php.
1. No changes to swig's internal C++ code is needed for perl.
2. Small changes to swig's internal C++ is needed for php.
I've included the modified Source/Modules/php.cxx file in its
complete form.
Note: I submitted a patch earlier that adds multi-valued (arrayed)
returns for php and I included svn diffs for php.cxx with that
patch. I've made further improvments so this file supercedes the
version I sent earlier.
After some thought, I think that sending full files is likely to be
better. But, if you'd prefer diffs, please let me know.
3. The existing Lib/intrusive_ptr.i file has been changed.
The new version is quite a bit smaller as a good portion of the
code has been put into language-specfic boost_intrusive_ptr.i
files.
4. The Lib/{php,perl5,python}/boost_intrusive_ptr.i have been added.
The perl5 and php files have been shown to work with an external
project (google's libkml). I've added a swig-local test cases for
all of the affected languages. See below for more.
5. The Lib/java/boost_intrusive_ptr.i typemap file has been changed.
swig already provided this file but from what I could tell, this
didn't offer intrusive_ptr support. Instead, it seemed to provide
boost shared_ptr support.
I've changed this file so that it follows the same basic pattern
used in the other boost_intrusive_ptr.i files. Eg, so that it
actually provides boost support.
7. The Examples/test-suite/li_boost_intrusive_ptr.i file has been
changed.
This language-specific file seemed to be written with java in mind,
and with shared_ptr support in mind, too. I've removed the code
that was specific to java and/or shared_ptr. I added comments,
etc.
8. The Examples/test-suite/java/li_boost_intrusive_ptr_runme.java file
has been changed.
I've removed the code that depended upon shared_ptr. I also added
comments, etc.
9. I've added these new files:
Examples/test-suite/php/li_boost_intrusive_ptr_runme.php
Examples/test-suite/perl5/li_boost_intrusive_ptr_runme.pl
Examples/test-suite/python/li_boost_intrusive_ptr_runme.py
These all follow the same basic form (and all are modeled on the
existing java test program).
10. Testing and status.
a. php
intrusive_ptr support for php works well with the google-libkml
project, a project external to swig that makes use of
intrusive_ptr and swig.
The swig-local test case that I've added also works without any
problems.
b. perl5
intrusive_ptr support for perl5 works well with google-libkml.
The swig-local test case I've added has two errors and both seem
to arise from the way that libraries are loaded. The rest of
the tests seem to work without any problems.
Specifically, these two lines at the top of the test file:
BEGIN \{ use\_ok\('li\_boost\_intrusive\_ptr'\) \}
require\_ok\('li\_boost\_intrusive\_ptr'\);
result in these failures:
not ok 1 - use li\_boost\_intrusive\_ptr; \# Failed test 'use li\_boost\_intrusive\_ptr;' \# at ./li\_boost\_intrusive\_ptr\_runme.pl line 6. \# Tried to use 'li\_boost\_intrusive\_ptr'. \# Error: Can't bless non-reference value at li\_boost\_intrusive\_ptr.pm line 21. \# Compilation failed in require at \(eval 3\) line 2. \# BEGIN failed--compilation aborted at ./li\_boost\_intrusive\_ptr\_runme.pl line 6. not ok 2 - require li\_boost\_intrusive\_ptr; \# Failed test 'require li\_boost\_intrusive\_ptr;' \# at ./li\_boost\_intrusive\_ptr\_runme.pl line 7. \# Tried to require 'li\_boost\_intrusive\_ptr'. \# Error: Attempt to reload li\_boost\_intrusive\_ptr.pm aborted. \# Compilation failed in require at \(eval 5\) line 2.
If you change use_ok/require_ok lines so that libraries are
loaded in a more normal fashion, these problems disappear. I'm
not sure why these errors occur and I haven't yet taken the time
to find out.
c. java
swig is able to generate code without problems and this code
compiles cleanly by the C++ compiler on my machine. Moreover,
the test runs to completion with all tests working as expected.
However, after all of the tests have been run, during what I'm
presuming to be the program shut-down phase, there is a core
dump. I believe the problem is because of multiply-deleted
objects, but I'm not entirely sure.
I do know that I've had to disable the explicit memory
management used the perl5/php/python typemaps. While this
follows the same basic approach used in other java typemaps,
this does seem counter-intuitive to me. I admit that I know
little about java's memory management. Perhaps someone with
more experience can have a look.
d. python
The code generated by swig doesn't compile. All of the failures
occur in the "swigregister" functions:
SWIGINTERN PyObject *Klass_swigregister(PyObject *SWIGUNUSEDPARM(self), PyObject *args) {
PyObject *obj;
if (!PyArg_ParseTuple(args,(char*)"O:swigregister", &obj)) return NULL;
SWIG_TypeNewClientData(SWIGTYPE_p_boost__intrusive_ptrT_Space__Klass_t, SWIG_NewClientData(obj));
return SWIG_Py_Void();
}
I'll continue to look into this however ... others who know
more about the python generator may be able to produce a fix
more quickly.
11. Documentation
I've added copius inline comments, but haven't produced
swig-specfic external/HTML documentation about intrusive_ptr
support yet.
12. Other typemap files
I submitted a patch to swig that adds/improves multi-valued
(arrayed) return for swig/php. Most of the changes were in
php-specfic typemap files, and I submitted the patch in the form
of svn diffs. For completeness, I've included the modified
Lib/php files in their complete form with this patch. Strictly
speaking, they're not necessary for intrusive_ptr support, but I
figure it would be useful to include them here:
Lib/php/php.swg
Lib/php/utils.i
Lib/php/phpkw.swg
Lib/php/typemaps.i
Lib/php/factory.i
Lib/php/globalvar.i
Lib/php/phppointers.i
Lib/php/std_string.i
Lib/php/director.swg
Lib/php/phprun.swg
Lib/php/const.i
David Fletcher Tuscany Design Automation, Inc.
frodo@tuscanyda.com 4800 Innovation Drive
Ft. Collins, CO 80525 USA
Hi David. This looks like it is a large amount of code to review and consequently a patch is essential. Please run diff on the original version and your version to create the diff. We have no idea what the original version is and if we were to just take the files as is, they might wipe out some commits since you took your snapshot of the code.
I had a quick look at the files, and the testing is comprehensive, which is great. I can't try this out though as it will take some effort to apply the patch without the diffs. You've clearly put a lot of work into this. I know it takes a ages to follow the memory management of smart pointer implementations and the interaction with the different languages. I also would like to sit down and understand and check this, but concentrate on it once all the basic issues in the patch are fixed.
One important thing, please resubmit with all copyrights removed and mention of licenses in the files. Going forwards we are making everything in the Lib directory and Examples directory free of copyright and licenses. A big patch will be going in the coming months to sort what is already there.
One last formatting bugbear: please follow the common formatting, eg :
%typemap(in) CONST CORE_TYPE * (SMART_PTR * smartarg = 0)
not
%typemap(in)
CONST CORE_TYPE * (SMART_PTR * smartarg = 0)
and
void addref(void) const {
not
void
addref(void) const {
With regard to the current Java implementation, it modifies SWIG to store a pointer to a shared_ptr of the underlying type instead of a pointer to a pointer to underlying type. I don't recall the details, but storing a pointer to an intrusive_ptr didn't look like it would work with regard to memory management. I'm sorry I don't remember what the supposed issue was, but if the approach you have taken shows no memory issues then it must be okay.
With regard to 10 - the testing, one important question that needs answering with the tests, is, are there any memory leaks at all and how do the tests prove this - is there a macro to turn on like with the shared_ptr testing? If there are any leaks then they need to be understood and fixed, as until they are, there could be something fundamentally wrong with the design.
For 10b - perl, you'll have to get some perl help from somewhere.
For 10c - java, these will need fixing and sound like there is a fundamental problem. The garbage collector will call the SWIG Java proxy object delete(), which will in turn call the destructor wrapper.
For 10d - python, some of the swigregister function code is controlled by the "smartptr" feature.
Can you please keep just this one SF patch open for this, so we can follow it all in one thread? I'll close the other ones you opened, namely
https://sourceforge.net/tracker/?func=detail&atid=301645&aid=2941089&group_id=1645
https://sourceforge.net/tracker/?func=detail&atid=301645&aid=2941704&group_id=1645
Refering to https://sourceforge.net/tracker/?func=detail&atid=301645&aid=2941704&group_id=1645, quite a bit of charm is required ;) This review will take quite some unpaid effort and I'd like to keep this time down. Ideally I'd like to see a fully working version in either one of Python, Java or C# as I know these modules best and they are the top most used modules in SWIG. I don't mind then reviewing another language module based on similar concepts as I can use one of these 3 that I know well as a reference. Unfortunately you only have PHP working. Any chance you can extend this to Python? I don't know enough about Perl to fix the module loading problems you are having and so Perl is bit of a non-starter. However, the changes you have made to Perl look familiar as it is a UTL language, but PHP is that much different that I can't start the review with PHP and I don't want to start a review that isn't working in the first place.
Thanks
William
David, can you please submit your patch as a normal open source patch? If you google for "how to create a patch", you'll see how to do it, something like 'diff -Naur original new', where original is latest trunk in svn. You'll probably need to add in some --exclude options. Please review the patch before submitting as when I looked at one of your last diffs it just showed all the lines being turned from unix to dos, which is not what I want to spend my time reviewing! You should check the patch minimally using patch --dry-run. What is expected by anyone looking at your patch is to simply run 'patch -p0 < file.patch' on a freshly checked out version of svn, then the files can be reviewed easily as I really don't have time to manually apply your current tarballs which require faffing around and then will still probably end up removing some recent commits. Thanks. William
David posted a new patch in 2948562 (now closed). Please keep future patches to this one thread. Thanks. I can't keep track of all these different threads. No response to my 2nd query on 2010-01-28 in this thread?
Yes, we really do want patches not modified files. If you send me a whole modified file (or a tarball of them), then I want to see what you changed, so the first thing I need to do is track down the original version you started from (which is fiddly to determine) and then generate a patch.
Since you should know the version you started from, please just send a patch in the first place! If you work from an SVN checkout (recommended) then "svn diff" will do that for you in a single command. If you are diffing two versions of an unpacked tarball then please make sure to use GNU diff's "-u" option so the patches have context.
Better still would be one patch per logical change. As I've said before, if you'd broken this down in the first place, we could probably have got most of this reviewed and committed by now...
No response, closing.