In the interests of actually getting these changes applied in a prompt and orderly fashion while allowing me to stay sane, can you please submit patches with a single purpose, in a separate ticket for each, with all related changes in a single patch file (not a tarball of patch-per-file for all sorts of different reasons which this seems to be).
And please be extra careful to remove debug lines and extraneous changes. The first file I looked at at was swig-diffs/Lib-php/const.i-diff which has this, which we clearly don't want:
+ /* const.i, tm1 */
If I have a single patch with a clear purpose which is obviously correct (and has test coverage if possible) then it's a no-brainer to apply. Repeat that for a few patches, and we're done! This also allows you to prioritise what you'd like to get reviewed first, in case there's work you want to build on.
If i have a tarball of random changes, some of which aren't relevant, some of which need further work, I have to try to unpick what relates to what, weed out what's bad and definitely not wanted, point out the problematic parts, apply the good parts, and try to keep track of the status of everything. That's a lot of pain and is going to take ages.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Nearly all of the changes I made are to provide support for multi-valued (arrayed) return. A very small portion of the change adds support to handle smart pointers, and this change depends in some part on support for multi-valued return.
Unfortunately, adding support for multi-valued return is fairly invasive in that it affects many of the PHP-specific typemaps. I don't see any way to break the patch into smaller pieces. If I could have done that, believe me, I would have. Each individual diff can be applied against the current version of swig to realize one complete patch. I created the diffs for each file rather than the entire set of changes because I figured that it would be easier to analyze each diff that way. If you'd prefer one large patch I can generate that.
I tried to document each of the changes in the README files to a fairly fine level. If the description of the changes aren't sufficient, please let me know.
In terms of the debug comments, I explained in the README files that this makes the task of finding and fixing problems much simpler. These are present because, in my experience, swig doesn't always emit source information in a reliable fashion. I explained in the README files - but it's worth mentioning again - that you are welcome to remove these lines if they are offensive, or to ask me to remove the lines and I'm happy to oblige.
In terms of testing, I have run the full PHP test suite and I've reported on the results. The README files show the tests that are failing. I've included my diagnosis as to why each test fails. If you need additional information, please feel free to contact me and I'll be happy to supply more information.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
PHP multi-valued return
In the interests of actually getting these changes applied in a prompt and orderly fashion while allowing me to stay sane, can you please submit patches with a single purpose, in a separate ticket for each, with all related changes in a single patch file (not a tarball of patch-per-file for all sorts of different reasons which this seems to be).
And please be extra careful to remove debug lines and extraneous changes. The first file I looked at at was swig-diffs/Lib-php/const.i-diff which has this, which we clearly don't want:
+ /* const.i, tm1 */
If I have a single patch with a clear purpose which is obviously correct (and has test coverage if possible) then it's a no-brainer to apply. Repeat that for a few patches, and we're done! This also allows you to prioritise what you'd like to get reviewed first, in case there's work you want to build on.
If i have a tarball of random changes, some of which aren't relevant, some of which need further work, I have to try to unpick what relates to what, weed out what's bad and definitely not wanted, point out the problematic parts, apply the good parts, and try to keep track of the status of everything. That's a lot of pain and is going to take ages.
Olly,
Nearly all of the changes I made are to provide support for multi-valued (arrayed) return. A very small portion of the change adds support to handle smart pointers, and this change depends in some part on support for multi-valued return.
Unfortunately, adding support for multi-valued return is fairly invasive in that it affects many of the PHP-specific typemaps. I don't see any way to break the patch into smaller pieces. If I could have done that, believe me, I would have. Each individual diff can be applied against the current version of swig to realize one complete patch. I created the diffs for each file rather than the entire set of changes because I figured that it would be easier to analyze each diff that way. If you'd prefer one large patch I can generate that.
I tried to document each of the changes in the README files to a fairly fine level. If the description of the changes aren't sufficient, please let me know.
In terms of the debug comments, I explained in the README files that this makes the task of finding and fixing problems much simpler. These are present because, in my experience, swig doesn't always emit source information in a reliable fashion. I explained in the README files - but it's worth mentioning again - that you are welcome to remove these lines if they are offensive, or to ask me to remove the lines and I'm happy to oblige.
In terms of testing, I have run the full PHP test suite and I've reported on the results. The README files show the tests that are failing. I've included my diagnosis as to why each test fails. If you need additional information, please feel free to contact me and I'll be happy to supply more information.
Multi-valued return works in PHP with current SWIG (tested with SWIG 4.0.2 and git master, using the
multivalue_runme.phpI added to git last month).