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)
         else \
             $(SWIG) -scilab $(SWIGOPT) $(SCILABOPT) -addsrc $(SRCS)
         fi \
     else \
         if test ! -z "$(INCLUDES)"; then \
             $(SWIG) -scilab $(SWIGOPT) $(SCILABOPT) -addcflag
         else \
             $(SWIG) -scilab $(SWIGOPT) $(SCILABOPT) $(INTERFACEPATH); \
         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"
(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:


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" {

- 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?