Menu
▾
▴
Re: [Swig-devel] Swig Scilab ready ?
|
From: Simon M. <sim...@sc...> - 2013-10-01 08:24:34
|
Le 18/09/2013 01:06, William S Fulton a écrit :
> On 06/09/13 18:57, William S Fulton wrote:
>> On 06/09/13 11:47, Simon Marchetto wrote:
>>> Le 05/09/2013 09:55, William S Fulton a écrit :
>
>>> Not overriding the test-suite swig & compile command
>>> lines (swig_and_compile_cpp,...) ?
>>> scilab & scilab_cpp targets are used only to compile the code. OK, we
>>> execute Scilab for this, because it has a simple command that does the
>>> job. We could do it by our own, it maybe be better, I don't know at
>>> this
>>> moment, but it would take time to change all this.
>>>
>>> I've introduced the get_swig_scilab_args to replace that following
>>> code:
>>> scilab: $(SRCS)
>>> if test ! -z "$(SRCS)"; then \
>>> if test ! -z "$(INCLUDES)"; then \
>>> $(SWIG) -scilab $(SWIGOPT) $(SCILABOPT) -addsrc $(SRCS)
>>> -addcflag $(INCLUDES) $(INTERFACEPATH); \
>>> else \
>>> $(SWIG) -scilab $(SWIGOPT) $(SCILABOPT) -addsrc $(SRCS)
>>> $(INTERFACEPATH); \
>>> fi \
>>> else \
>>> if test ! -z "$(INCLUDES)"; then \
>>> $(SWIG) -scilab $(SWIGOPT) $(SCILABOPT) -addcflag
>>> $(INCLUDES) $(INTERFACEPATH); \
>>> else \
>>> $(SWIG) -scilab $(SWIGOPT) $(SCILABOPT)
>>> $(INTERFACEPATH); \
>>> fi \
>>> fi
>>>
>>> This code is hard to read, and more hard to maintain, and we have it
>>> twice, in the two targets.
>>> Moreover, I had to add another option 'outdir' (to build in separate
>>> directory, I couldn't make swig work without it, having the message
>>> "Unable to open ...wrap.cxx", maybe you have an idea on this).
>>> So i would have a tree of if, with 8 possible combinations. And we may
>>> add other options in the future.
>>> OK, the $(eval, ...) is not a simple syntax, but globally the result
>>> looked much more readable & maintainable to me.
>>> The macro has a name so we know what it does. And to add an option, it
>>> is just simple as this:
>>> ifdef OUTDIR
>>> SWIG_SCILAB_ARGS += -outdir "$OUTDIR"
>>> #endif
>>> (by the way, ifdef ... looks simpler to me than theseif test ! -z
>>> "$... )
>>>
>>> But I am not an expert in shell. And consistency is important to me. So
>>> what can we do ?
>>>
>> It all seems a lot more complicated than elsewhere. Model it on Java
>> which uses -outdir. And if you need to use -addsrc, add it to the
>> SWIGOPT, eg like this in test-suite/java/Makefile.in:
>>
>> SWIGOPT += $(JAVA_PACKAGEOPT)
>>
> These makefile changes still need doing.
>
> Here is a list of the remaining issues I can see. This concludes my
> review for the moment. I just want to look at the typemaps in more
> detail next, then probably we can think about merging into master once
> the issues mentioned including the ones below are addressed.
>
> - The banner is missing in builder.sce. You'll need to code something
> up like JAVA::emitBanner().
>
> - typedef int SciObject; => All C/C++ symbols introduced by SWIG in
> the wrapper code should have Swig or SWIG as a prefix. Sci as a prefix
> ought to be reserved for symbols in the Scilab headers.
> There are a few of these, such as SciSequence_InputIterator should be
> SwigSciSequence_InputIterator, SciSwigIterator_T should be
> SwigSciIterator_T. Use 'grep -i sciswig' to find them or look at the
> symbols in a compiled shared object. Also there are other global
> variables such as fname and outputPosition which should be prefixed.
>
> - This include:
> #include "stack-c.h"
> It would be better to keep all the scilab #includes together, so I
> would move this further up the file.
>
> - extern "C" should be within the __cplusplus macro:
> #ifdef __cplusplus
> extern "C" {
> #endif
>
> - Should fname in all the wrappers, such as:
> int _wrap_Square_perimeter(char *fname, unsigned long fname_len) {
> not be const char *, or is that the correct convention?
>
> - Does Scilab support multiple threads? The use of globals such as
> fname and outputPosition is not thread safe. The goal of SWIG is to
> not remove thread safety if it already exists in the library being
> wrapped.
>
> - /* functionReturnTypemap */ comment in generated code should be
> removed.
>
> - Test-suite...
> constructor_copy.i => I've restored some lost code in this testcase
> (additional constructors) and committed it. If it breaks anything in
> scilab, the language module itself will need fixing.
> ignore_template_constructor.i => Scilab should work with this test.
> Let's see what breaks.
> Examples/test-suite/scilab/aggregate_runme.sci => we can change the
> name of 'move' to something else in the .i file (let's do when the
> merge to master is complete).
>
> - What the situation about identifiers being too long, what are the
> restrictions? Can you document details in Scilab.html?
>
> - There is some usage of mprintf for errors in the test-suite
> (swigtest.start). Errors should print to stderr - can this be done?
>
> William
>
Wiliam,
Last week, I've fixed almost all the issues you mentioned (including the
test-suite & example makefiles, they are based now on Java makefiles).
What mostly remains in my todo list is about typemaps:
- STL typemaps: I would like to support (or to add the possibility to
support) vector<enum>, and reject properly (without compilation error)
vector<class> (class do not exist in scilab) (vector<class*> are
supported). These issues are somewhat related to constructor_copy and
ignore_template_constructor tests.
- double/int typemaps : situation is totally confused in the code by
now. What I would like : Scilab main type is double, so it would be
convenient to convert by default int to/from double. Otherwise, the user
will have to cast -everywhere- in the Scilab script. But I want this
behaviour be disabled if needed (as it is not a standard programming
behaviour), by a preprocessor directive or a SWIG feature (I prefer the
first option for performance reason).
I would be glad to have your opinion on that topics.
After these two important things will be fixed, I could switch serenely
to the documentation.
Regards,
Simon
|