Hi William,

Sorry for delay, here are my answers.

configure.ac:
- scilab is not disabled if the header files are not found.
OK
- Does scilab not use pkg-config or a scilab-config? Most of the modern
scripting language do so that it is easy to find the location of headers
and binaries etc.
It seems we support pkg-config. I will use it if possible.


Examples/Makefile.in:
- uses abspath... please change to relative path
Scilab changes of directory when it builds (with make)....
If I give him a relative path, the compilation fails.
I don't like this too,  but I don't really have the choice.


- why do the scilab and scilab_cpp targets check for builder.sce? Don't
the examples/test-suite always generate builder.sce? Does it have to set
MAKEFLAGS, what is the default parallelisation for the builder?
We can remove the check.

I have to override the MAKEFLAGS, otherwise the Scilab make is done in parallel and fails. I wrote some emails about this when I integrated Scilab into Travis. Despite my efforts, I could not find other way.

- the scilab_run target shouldn't have an if statement, it should always
be run like the other languages

OK
- the use of eval and get_swig_scilab_args seems overly complex, can't
it just use a simple if?

I hate duplicate code, but I can replace it by some ifs in targets scilab and scilab_cpp. These duplicate ifs won't be far each other, so it is not so hard to check.


- the swig executable invocation does not use -o
Need a little analysis on this.
- each of the individual example Makefiles sets TARGET to the name of
the generated file, it should be the module name.
Need to see this also

- the contents of INCLUDES in test-suite/scilab/Makefile.in is different
to normal, I'm not sure there is a good reason for this, please restore
to normal

Should Examples/scilab/matrix2/README be checked in?
It will be removed.

The constants, enum, template and variable examples have deviated from
the versions in the other languages. Can you change them to use the
identical source input files as say Java/Python. One day I would like
common examples source for all different languages.

I understand this for consistency.
But examples should also illustrate specific features of each language, shouldn't they ?
Where can the user play with the feature with scilab_const, for example ?


Hi Simon

Any response to the above points? I have some new comments...

a) I suggest you change using 999 in Scierror to a macro, such as:
#ifndef SWIG_SCILAB_ERROR
#define SWIG_SCILAB_ERROR 900
#endif
so that users can easily modify the error value via -D or %begin. The value you have chosen could all too easily be common value.
How about using the SWIG error codes too from a base value of say 900 and use:
SWIG_Scilab_Error(int code, const char *msg)
{
  Scierror(SWIG_SCILAB_ERROR - code, _("SWIG/Scilab: %s: %s\n"), SWIG_Scilab_ErrorType(code), msg);
}

Error codes are defined in Scilab headers. 999 is a common error code in Scilab, but should be replaced by a macro.
And I agree that the user should choose the error code value to discriminate between SWIG and Scilab.


b) Parameter variable names are using _ as a prefix in the runtime code, please remove the prefix, it isn't a convention we use.

OK
c) What is the point of the multiple %rename in the li_std_string.i testcase? Is this because Scilab can't handle very long identifiers?

Surely !
d) Parameter variable names begin with _ in the Lib/scilab/* files. Can you drop the leading underscore please? By convention we only use this convention for member variables (and even then, not very often).

OK
e) Can you change the following fragment name:
%fragment("include_for_uintptr", "header") {
%#include <stdint.h>
}
to be "<stdint.h>" as this is how we name the fragments which are for #include.

OK
f) Can you add in the following type of lines in .travis.yml:
         ["scilab"]="-Werror -std=gnu89 -pedantic -fdiagnostics-show-option -Wno-long-long -Wreturn-type"
and fix any warnings so it works (you may drop -pedantic if there are problems in the scilab headers):

OK, I'll check this.
g) I think Lib/scilab/boost_shared_ptr.i is missing parts of a fix that went into 8c24e2ca749a23184ea32696201a26bce711ff5f (out and varout typemap changes)
In fact, I didn't have a look on Boost shared pointers at all. I'll check this fix.

h) Can you change stl.i to pull in the same set of headers as the other languages, no more, no less!

Even std::deque, std::multiset, etc... do work and are tested, in the case of Scilab ? OK

i) swig_this and swig_ptr are functions and should hence use SWIG instead of swig as the prefix.

I picked up "swig_this" name from another language (Octave). OK
I'm nearing the end of my review. I'll probably post one more set of comments.

Good news ! And thanks for your review.

William