I've done all of the changes.

Unsigned integers are handled quite differently to signed integers wrt conversion to double. I don't see any logic in this, I think they should be handled the same, that is, unsigned and signed converted to and from doubles equally. Can you elaborate why this shouldn't be done?

Done

Named typemaps use upper case as parameter names in SWIG.
Can you modify the typemaps included by matrix.i to use upper case (and correct the html docs too). You could then drop the 'matrix' prefix, eg:
matrixIn => IN

Done

Can you also make the option names a bit more readable, I suggest changing:

-addcflag    => -addcflags
-addldflag   => -addldflags
-addsrc      => -addsources
-vbl <level> => -buildverbosity <level>
-intmod      => -internalmodule
-ol <library name> => -outputlibrary <name>

Done

Simon


Le 12/06/2014 12:22, Simon Marchetto a écrit :

Le 12/06/2014 08:35, William S Fulton a écrit :
On 10/06/14 16:54, Simon Marchetto wrote:
When do you want to make the release ?

As soon as the Scilab module is merged into master. Hopefully in a couple of weeks time, depending on what is found during review and if you have the time to address issues as they are found.


I've read the Scilab.html chapter and have a few initial comments:

Building the module section:
- There is no ./scilab-cli script
Done => replaced by "scilab"
- Can you describe more what the point of libexample.c is? Why is it needed in addition to the _wrap.c file?

Done => added the following short explanation "used by Scilab at run time to link each module new declared function in Scilab to the related wrapped C/C++function (ex: foo() in Scilab is routed to the C/C++ function wrap_foo())"

I will add at the end of the doc some other references to Scilab documentation (describing Scilab external modules). You may find some details there.
Why is the file always called loader.sce ? In the other languages, you get a file named after the module name, so example_loader.sce would be more appropriate. Likewise for builder.sce.

Historical reason. I agree with you. I have to see if we can change it in the future version of Scilab.
The output of 'swig -scilab -help' does not match the documented command line options. Can you also format this output to line up like the other options?

Can you also make the option names a bit more readable, I suggest changing:

-addcflag    => -addcflags
-addldflag   => -addldflags
-addsrc      => -addsources
-vbl <level> => -buildverbosity <level>
-intmod      => -internalmodule
-ol <library name> => -outputlibrary <name>

"Note: in fact, it is not necessary to include the library typemaps.i, this one is included by default. " This is inconsistent with the other language, please change it to not include by default.

=> I can run the example I gave in the documentation in Scilab and in Python, without including typemaps.i.
Is it because this example does not need typemaps.i ? Or because typemaps.i is included by default ?
In any case, I did not intend to include it by default. I did a search now, I couldn't find any such include in Lib/scilab, but maybe I missed it.


Unsigned integers are handled quite differently to signed integers wrt conversion to double. I don't see any logic in this, I think they should be handled the same, that is, unsigned and signed converted to and from doubles equally. Can you elaborate why this shouldn't be done?

OK I will fix it.
Named typemaps use upper case as parameter names in SWIG.
Can you modify the typemaps included by matrix.i to use upper case (and correct the html docs too). You could then drop the 'matrix' prefix, eg:
matrixIn => IN

OK
In the docs: "Scilab is supported from version 5.3.3". but configure.ac checks for 5.3 or higher.

In configure:

  checking for scilab... scilab
  configure: Scilab version: 5.2.0.1266391513
  checking for Scilab version is 5.3 or higher... no
  checking for Scilab startup options... -nwni -nb
  checking for Scilab header files... /usr/include

Done => configure checks 5.3.3 instead of 5.3.
there is no need to do the remaining checks if the version number is not high enough.

Done

William