Le 18/09/2013 01:06, William S Fulton a écrit :
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


Hello William,

Agree with all this.

About:

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


Scilab convention is char *....

- Does Scilab support multiple threads?

No Scilab is not threadsafe, in any case, in 5.x.

- What the situation about identifiers being too long

All identifiers must be 24 chars max long. Again, in Scilab 5.x. Scilab has a very long history....
About documentation, I will complete it soon.

Simon Marchetto