search

jaeandersson / swig

SWIG is a software development tool that connects programs written in C and C++ with a variety of high-level programming languages.
http://www.swig.org
Other
23 stars 19 forks source link

Module support #43

Closed dcb210 closed 9 years ago

dcb210 commented 9 years ago

Seems to work well with change to swigPtr. Running "make check-matlab-test-suite RUNPIPE=>>/tmp/xxx" 81 Passing 46 Failed = contract failed cpp_enum failed default_constructor failed director_abstract failed director_classic failed director_detect failed director_enum failed director_extend failed director_nested failed director_unroll failed exception_order failed friends failed li_carrays failed minherit failed namespace_typemap failed null_pointer failed overload_extend failed overload_rename failed overload_simple failed overload_template failed preproc_constants failed reference_global_vars failed return_const_value failed smart_pointer_extend failed smart_pointer_member failed smart_pointer_multi failed smart_pointer_multi_typedef failed smart_pointer_not failed smart_pointer_overload failed smart_pointer_rename failed smart_pointer_simple failed smart_pointer_templatevariables failed smart_pointer_typedef failed struct_value failed template_typedef failed template_typedef_cplx failed template_typedef_cplx2 failed voidtest failed director_string failed li_std_vector_enum failed li_carrays failed li_cmalloc failed nested_structs failed overload_extend failed unions failed imports failed

It looks like directors needs some work. I've personally never tried directors. Others need to be investigated.

jaeandersson commented 9 years ago

I missed that you had made this pull request. I'll try it out. On a separate note: I'm thinking that the type information would be best stored in a separate mex-file, common to all modules. There should be no problem creating such a base, given that we already have a SwigRef class common for all modules.

But this is something that can be added later, it's in no way a prerequisite for implementing this.

jaeandersson commented 9 years ago

Pull request merged! @dcb210 - really great contribution!

@jgillis - could you confirm that the director support didn't get broken by this?

dcb210 commented 9 years ago

Did director support work previously? It seems like director support is now broken. I’ll see if I can submit a fix.

On Sep 4, 2015, at 6:28 AM, Joel Andersson notifications@github.com wrote:

Pull request merged! @dcb210 https://github.com/dcb210 - really great contribution!

@jgillis https://github.com/jgillis - could you confirm that the director support didn't get broken by this?

— Reply to this email directly or view it on GitHub https://github.com/jaeandersson/swig/pull/43#issuecomment-137698427.

jaeandersson commented 9 years ago

@jgillis Implemented support for directors and as far as I know, it did work (at least in our project). Not sure if the unit tests passed though.

jgillis commented 9 years ago

There was one director unittest that passed: director_basic. Does this pass still?

dcb210 commented 9 years ago

Yes. The following fail: director_abstract failed director_classic failed director_detect failed director_enum failed director_extend failed director_nested failed director_unroll failed

On Sep 4, 2015, at 7:26 AM, jgillis notifications@github.com wrote:

There was one director unittest that passed: director_basic. Does this pass still?

— Reply to this email directly or view it on GitHub https://github.com/jaeandersson/swig/pull/43#issuecomment-137710345.

jaeandersson commented 9 years ago

Good, so it's not more broken than before:)

dcb210 commented 9 years ago

I’m not sure why you would want to create a separate mex file just for type information. The current approach, which mimics other languages wrapped by SWIG, works seamlessly whether you have a single module or multiple modules. Adding another mex file means an additional compilation step which, to my knowledge, no other SWIG wrapped language requires. The only benefit is that you could permanently lock (using mexLock()) just the one module that stores the type information and allow all other mex files to be unloaded by Matlab when all objects have been deleted. Is this really a benefit? What are the performance/memory benefits of allowing Matlab to unload a module? I’m not sure this benefit is worth deviation from the norm.

On Sep 4, 2015, at 6:02 AM, Joel Andersson notifications@github.com wrote:

I missed that you had made this pull request. I'll try it out. On a separate note: I'm thinking that the type information would be best stored in a separate mex-file, common to all modules. There should be no problem creating such a base, given that we already have a SwigRef class common for all modules.

But this is something that can be added later, it's in no way a prerequisite for implementing this.

— Reply to this email directly or view it on GitHub https://github.com/jaeandersson/swig/pull/43#issuecomment-137694505.

jaeandersson commented 9 years ago

I'm not sure that it's needed. I'm just a bit worried about using raw pointers because MATLAB tends to free memory when you least expect it. I might be wrong about this and now that the module is more complete, it might not be an issue anymore. I'd keep the current design in place - it's both efficient and clean - and only refactor it if we end up getting memory issues.

dcb210 commented 9 years ago

I think the current approach ought to work well as long as the end-user doesn’t inadvertently change the SwigPtr. In fact, I tried to make SwigPtr protected to make this possibility less likely. Sadly, Matlab is smart enough to know when we are trying to alter SwigPtr from the mex file and it causes runtime errors. Perhaps someone can think of a workaround?

On Sep 4, 2015, at 9:59 AM, Joel Andersson notifications@github.com wrote:

I'm not sure that it's needed. I'm just a bit worried about using raw pointers because MATLAB tends to free memory when you least expect it. I might be wrong about this and now that the module is more complete, it might not be an issue anymore. I'd keep the current design in place - it's both efficient and clean - and only refactor it if we end up getting memory issues.

— Reply to this email directly or view it on GitHub https://github.com/jaeandersson/swig/pull/43#issuecomment-137744947.

jaeandersson commented 9 years ago

Sadly, Matlab is smart enough to know when we are trying to alter SwigPtr from the mex file and it causes runtime errors.

Yes, I know. That's the reason why it isn't protected in the first place. But I think this is minor. I don't think users are likely to change it.