Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Matlab update #96

Open
wants to merge 3,104 commits into
base: matlab
Choose a base branch
from
Open

Conversation

KrisThielemans
Copy link

@KrisThielemans KrisThielemans commented Jul 30, 2021

See swig#2058

ojwb added 30 commits May 3, 2021 16:00
Without inventing a SWIG/PHP-specific mechanism, we can't really
finalise objects in the way the testcase expects, so adjust the
testcase minimally so we avoid triggering C++ undefined behaviour
(use-after-free).
With modern PHP it only works with the CLI version of PHP, so it's
better to direct users to load the extension via "extension=" in
php.ini.

Suggested by ferdynator in swig#1529.
This works for PHP >= 7.2 and is the recommended method now as it
avoids having to specify a filename which varies between platforms.
This has been in the code for a really long time, and doesn't seem
to be required now.  It's not documented by PHP as something we
need to do, and the value seems to always be NULL at this point
already.
We can't safely lookup the Exception class entry at MINIT time, but we
can just use zend_ce_exception instead, which will be a bit faster too.
It's now only generated if something to put in it is specified via:

%pragma(php) include=...

or

%pragma(php) code=...
Since PHP 8.0 these now give an error.
Previously this relied on getting all known classes/functions/etc
when it was loaded, and then again after the PHP module being
tested was loaded.  This approach no longer works now we've
stopped loading modules using dl(), so use ReflectionExtension
instead to get information about a specific extension.

This is likely also faster than wading through lists including
everything predefined by PHP.
The updated tests.php is case sensitive.
We always include zend_exceptions.h via phprun.swg.
Eliminate redundant and unused includes.

Only include the minimum headers needed before the PHP_MAJOR_VERSION
check in case future PHP versions remove some of the headers we
include.
SWIG_SetPointerZval() now checks if the passed zval is an object,
so use ZVAL_UNDEF() before in cases where we create a zval to pass.
Sorry, failed to undo this temporary change before merging the
gsoc2017-php7-classes-via-c-api branch.
The calls to the parent class' magic __get, __set and __isset methods
weren't getting the prefix.
The underlying wrapper function is now always named using
ZEND_NAMED_FUNCTION even if it's a method (in PHP a function and
a method only differ in how they're used).
Add some missing entries, remove some long obsolete entries (from
the "ming" extension for generating SWF files, which was split
out from PHP core in 2008), and entry for "static" as a reserved class
name (`static::` is used for late static bindings, but attempting to
name a PHP class `static` fails because `static` is a keyword and we
also list it as such).
PHPCN(x) does a string compare of x with the lower-cased class name,
so x needs to be in lowercase or else the entry has no effect.  The
entries for TRUE, FALSE and NULL weren't working as a result.
KrisThielemans and others added 11 commits December 21, 2021 11:31
I have enabled examples that worked fine, but didn't attempt others
Move C++ comment testing into here.
See 7a9bf33.
With C++ comments changed to C comments, these are now identical to
the two cases just above, aside from the `2` suffix on the names.

Follow-on for swig#2027.
This serves as a regression test for
https://sourceforge.net/p/swig/bugs/1339/ which was presumably fixed by
the change to use PHP's C API to wrap classes.
We've dropped support for the old V8 versions which lacked version
macros, and SWIG_V8_VERSION now gets automatically defined by
Lib/javascript/v8/javascriptruntime.swg which will #undef it first if
it's already defined.
SwigRef.subsref used an empty () index, generating warnings with
recent Octave. This was superfluous so I've removed it.

Fixes jaeandersson#99
@KrisThielemans
Copy link
Author

I've reviewed all Octave problems and fixed things related to this PR and created issues for things which are not related to this PR. @jaeandersson I therefore believe that this PR considerably advanced the status and in my opinion should be merged :-)

Here's the detail.

I get a ton of warnings like

Warning: function ./+li_math/atan.m shadows a built-in function

now documented in #101 as unrelated to this PR

  1. With Octave 6.4.0 (but not with 4.2.2) I see warnings like
warning: 'uint64 matrix' object indexed with empty index list

now documented in #99 and fixed in b3594ff

  • unions.ctest and imports.multicpptest fail as Octave doesn't have import yet.

It seems that these were overlooked in b5e75d1. Now documented in #100 as this is not new to this PR.

  • director_default.cpptest` failure

This is actually an old one #69 and therefore not related to this PR

  • multi_import.multicpptest and template_typedef_import.multicpptest fails
...
ERROR: multi_import_aMEX: Cannot allocate pointer

I've created #102. Note that current matlab branch (i.e. before this PR) fails these tests before it ever gets there due to #95

error: subsref: unknown method or property: testx
error: called from
    multi_import_runme at line 7 column 1

@KrisThielemans
Copy link
Author

Fixes #95
Fixes #85
Fixes #38
Fixes #9 (@jaeandersson @traversaro please check the new manual!)

@KrisThielemans
Copy link
Author

@jaeandersson I think this is as far as I want to go with this PR. Lots of other things to do of course, but no point making this PR even larger than it is already. Please merge (as I cannot).

@KrisThielemans
Copy link
Author

Apologies @jaeandersson for pinging again, but if you have no time to merge this, I suggest that we move further development to my fork, or that you give me write permissions here such that I can merge.

@jaeandersson
Copy link
Owner

jaeandersson commented Jan 24, 2022

Apologies @jaeandersson for pinging again, but if you have no time to merge this, I suggest that we move further development to my fork, or that you give me write permissions here such that I can merge.

It is me who should apologize - I've really struggled to find time for this among my other commitments and I should have recognized that early on and not kept you guys hanging. Lets move the developments to your fork. I don't mind giving you permissions to this fork, but I think it would create confusion to do it in my fork since if I can not reproduce the results which I am not able to do right now. My (commercial) MATLAB license is for R2015b and on OSX, so it's a bit tricky to get everything working. Also using your fork means that you're getting proper credit for the work.

I will try to do some more testing on my end. I'm especially interested in getting Octave to work well. But I can just make a pull request to your branch if there are some minor changes.

* Update ci.yml
* Update testflags.py
* Use MATLAB tests to use ubuntu-18.04 to avoid LD_PRELOAD MATLAB workaround
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.