From: Simon M. <sim...@sc...> - 2013-08-26 15:52:08
|
William, As you said, Scilab builds look better on Travis. Yes, there is still a potential timeout issue on the test suite (i will find a way to fix this) but the most important is the code, isn't it ? Did you have a look on our code, do you validate it ? Do you think we can merge soon in master ? What else does it need for that ? Does documentation have to be written first, etc.. ? Best regards, Simon Marchetto |
From: William S F. <ws...@fu...> - 2013-08-29 06:26:00
|
On 26/08/13 16:51, Simon Marchetto wrote: > William, > > As you said, Scilab builds look better on Travis. > > Yes, there is still a potential timeout issue on the test suite (i will > find a way to fix this) but the most important is the code, isn't it ? Future maintenance is way more important than a dump of code. I feel strongly about this given the number of unmaintainable aspects of SWIG and the disappearance of language maintainers over time. I feel that having a passing regression test-suite is a necessary requisite for attempting any modification of the SWIG internals. I'll try see why parallel make solution for the timeout issue does not work for Scilab when I review, if you havn't worked it out sooner. > Did you have a look on our code, do you validate it ? Do you think we > can merge soon in master ? Thanks for your efforts to get it to the current state to work on Travis. I'll do the review next. > What else does it need for that ? Does documentation have to be written > first, etc.. ? A reasonable level of documentation is needed. Consider you are a first time user. If the documentation isn't good enough to successfully wrap a project for Scilab, then it isn't worth having Scilab as a SWIG target. Normally this doesn't require much documentation effort as there isn't that much that is language dependent. I havn't looked at the docs for Scilab yet, but will do during the review. William |
From: William S F. <ws...@fu...> - 2013-08-31 07:50:24
|
On 29/08/13 07:25, William S Fulton wrote: > On 26/08/13 16:51, Simon Marchetto wrote: >> William, >> >> As you said, Scilab builds look better on Travis. >> >> Yes, there is still a potential timeout issue on the test suite (i will >> find a way to fix this) but the most important is the code, isn't it ? > Future maintenance is way more important than a dump of code. I feel > strongly about this given the number of unmaintainable aspects of SWIG > and the disappearance of language maintainers over time. I feel that > having a passing regression test-suite is a necessary requisite for > attempting any modification of the SWIG internals. > > I'll try see why parallel make solution for the timeout issue does not > work for Scilab when I review, if you havn't worked it out sooner. Why in Examples/test-suite/scilab do you pipe 'exit(1)' into scilab like this: env LD_LIBRARY_PATH=.:$$LD_LIBRARY_PATH $(RUNTOOL) echo 'exit(1)' |$(SCILAB) -nwni -nb -f $(srcdir)/ I ran without this piping and it worked okay. Could you briefly explain the conf cache used in the test-suite? > >> Did you have a look on our code, do you validate it ? Do you think we >> can merge soon in master ? > Thanks for your efforts to get it to the current state to work on > Travis. I'll do the review next. I have been doing this and have just committed a bunch of changes. Other comments to follow... > >> What else does it need for that ? Does documentation have to be written >> first, etc.. ? > A reasonable level of documentation is needed. Consider you are a first > time user. If the documentation isn't good enough to successfully wrap a > project for Scilab, then it isn't worth having Scilab as a SWIG target. > Normally this doesn't require much documentation effort as there isn't > that much that is language dependent. I havn't looked at the docs for > Scilab yet, but will do during the review. The docs are need something on handling C++. Can you add that in? Minimum version required needs documenting. What is the real minimum version? I tried 5.2 which configure accepts and the first example failed. Do you expect 5.2 to work? As you may have noticed in my previous email to the list, I'd like to target Scilab inclusion for swig-3.0.0. William |
From: Simon M. <sim...@sc...> - 2013-09-02 16:11:01
|
Le 31/08/2013 09:50, William S Fulton a écrit : > Why in Examples/test-suite/scilab do you pipe 'exit(1)' into scilab > like this: > env LD_LIBRARY_PATH=.:$$LD_LIBRARY_PATH $(RUNTOOL) echo 'exit(1)' > |$(SCILAB) -nwni -nb -f $(srcdir)/ > > I ran without this piping and it worked okay. > > Could you briefly explain the conf cache used in the test-suite? You're right, we can remove the exit(1), I don't know the purpose of it, and it works without. Configure cache is used during a test when Scilab builds the module generated by Swig (with the Scilab function 'ilib_build'), with a standard configure and make. As all the tests use the same build configuration, the configure cache speeds up the test-suite by 10~20%. For this, it sets a configure cache file location in a configure site file (which location is given by CONFIG_SITE environment variable). I'm not sure a test-suite should modify its environment like this, and it is not a very elegant code. It was much a workaround for the test-suite on Travis, and also because Scilab 5.3.3 is out of my hands. So I am working on the parallelization of test-suite, I have a good hope it will work very soon, but I haven't finished yet. I will remove the configure cache then. > I have been doing this and have just committed a bunch of changes. > Other comments to follow... > Thanks, I merged that changes with my last improvements (the 'scilab:const' feature, quite same as 'java:const'), but i merged not in the right way, sorry for that.... > The docs are need something on handling C++. Can you add that in? > > Minimum version required needs documenting. What is the real minimum > version? I tried 5.2 which configure accepts and the first example > failed. Do you expect 5.2 to work? I have to document the C++ support, this among many other changes. About the minimal version, I have supposed it was 5.2 (as Scilab API comes in this version), but I haven't tested it, thanks for that. I will change the minimal version to 5.3.3, as it is also the oldest available version. Again, thanks. Simon |
From: William S F. <ws...@fu...> - 2013-09-02 17:16:46
|
On 02/09/13 17:10, Simon Marchetto wrote: > Le 31/08/2013 09:50, William S Fulton a écrit : > Configure cache is used during a test when Scilab builds the module > generated by Swig (with the Scilab function 'ilib_build'), with a > standard configure and make. > As all the tests use the same build configuration, the configure cache > speeds up the test-suite by 10~20%. > For this, it sets a configure cache file location in a configure site > file (which location is given by CONFIG_SITE environment variable). > > I'm not sure a test-suite should modify its environment like this, and > it is not a very elegant code. It was much a workaround for the > test-suite on Travis, and also because Scilab 5.3.3 is out of my hands. > > So I am working on the parallelization of test-suite, I have a good hope > it will work very soon, but I haven't finished yet. > I will remove the configure cache then. > Ah good. Parallel make really should work. I got some weird errors when trying parallel make, is it due the state being held by the cache or something else? William |
From: Simon M. <sim...@sc...> - 2013-09-03 17:18:35
|
Le 02/09/2013 19:16, William S Fulton a écrit : > On 02/09/13 17:10, Simon Marchetto wrote: >> Le 31/08/2013 09:50, William S Fulton a écrit : > >> Configure cache is used during a test when Scilab builds the module >> generated by Swig (with the Scilab function 'ilib_build'), with a >> standard configure and make. >> As all the tests use the same build configuration, the configure cache >> speeds up the test-suite by 10~20%. >> For this, it sets a configure cache file location in a configure site >> file (which location is given by CONFIG_SITE environment variable). >> >> I'm not sure a test-suite should modify its environment like this, and >> it is not a very elegant code. It was much a workaround for the >> test-suite on Travis, and also because Scilab 5.3.3 is out of my hands. >> >> So I am working on the parallelization of test-suite, I have a good hope >> it will work very soon, but I haven't finished yet. >> I will remove the configure cache then. >> > Ah good. Parallel make really should work. I got some weird errors > when trying parallel make, is it due the state being held by the cache > or something else? > > William The error message I had on parallel make was something like "read processus pipe: no such file". I couldn't figure out what was going on exactly. But this is past : the test-suite can run now successfully in parallel :-) For this I forced the Scilab make to work in serial. After that, I had to parallelize test-suite build & run. There was indeed conflicts between concurrent tests, because they build & run on the same folder, using same files. Now each test builds in its own subdirectory. The commit will come soon. I am impatient to test it on Travis. Regards, Simon |
From: Simon M. <sim...@sc...> - 2013-09-04 16:38:40
|
Le 03/09/2013 19:18, Simon Marchetto a écrit : > > The error message I had on parallel make was something like "read > processus pipe: no such file". I couldn't figure out what was going on > exactly. > > But this is past : the test-suite can run now successfully in parallel > :-) > > For this I forced the Scilab make to work in serial. > After that, I had to parallelize test-suite build & run. There was > indeed conflicts between concurrent tests, because they build & run on > the same folder, using same files. Now each test builds in its own > subdirectory. > > The commit will come soon. I am impatient to test it on Travis. > > Regards, > > Simon > Hello William, Done ! As you probably have seen, the test-suite runs now in less than 20 minutes, with "-j4" :-) I've also fixed the Scilab build script return code, so that Travis status is correct. Simon |
From: William S F. <ws...@fu...> - 2013-09-05 07:55:36
|
On 04/09/13 17:38, Simon Marchetto wrote: > Le 03/09/2013 19:18, Simon Marchetto a écrit : >> >> The error message I had on parallel make was something like "read >> processus pipe: no such file". I couldn't figure out what was going on >> exactly. >> >> But this is past : the test-suite can run now successfully in parallel >> :-) >> >> For this I forced the Scilab make to work in serial. >> After that, I had to parallelize test-suite build & run. There was >> indeed conflicts between concurrent tests, because they build & run on >> the same folder, using same files. Is SWIG generating the same output file names? It should not, so if that is the problem, this needs fixing on the SWIG side. If concurrent instances of Scilab can't read the same input files, then I would organise to get Scilab fixed as more than one instance of the same tool should be able run concurrently if the outputs are different. >> Now each test builds in its own >> subdirectory. >> >> The commit will come soon. I am impatient to test it on Travis. >> >> Regards, >> >> Simon >> > > Hello William, > > Done ! > As you probably have seen, the test-suite runs now in less than 20 > minutes, with "-j4" :-) This is good progress, thanks. Re the Examples makefile changes, I would prefer it to be a lot more like the other languages, because I end up having to maintain this stuff and it is so much easier if they are consistent. Can you put INCLUDE back to how it was? The scilab and scilab_cpp targets invoke scilab, they should only be compiling code, not running anything. Also why have the get_swig_scilab_args and get_output_dir macros? As you are using separate directories for the test-suite, did you notice that Java and C# take this approach and was there something wrong with the techniques used in there? If you think the makefiles can be improved, I'd like it as a patch across all languages - we havn't used $(eval $(call ...)) to date, I'm still trying to work out what exactly it is doing. Makefile macros are great obfuscators and generally I try to minimise their use as readability is way more important. > I've also fixed the Scilab build script return code, so that Travis > status is correct. > The constants example needed fixing and didn't exit with a bad exit code: SWIG/Scilab Error : RuntimeError Contract violation: require: (arg1>=0)&&(arg2>=0)SWIG/Scilab Error : RuntimeError Contract violation: require: (arg1>=0)checking Examples/scilab/enum I havn't checked yet your latest patches, but if this isn't sorted, please add to your TODO list. Examples/scilab/enum => for consistency, can you remove scilabconst_example.h and put the code in it into the example.i or example.h file. Also use example.cxx not example.cpp. One day I would like to create a common example framework. You probably have enough or even too many examples. I'd prefer you to test any further changes using the test-suite as the examples aren't very good for regression testing. Could you arrange to have Scilab patched for the next version to get rid of the completely pointless crud it is output each time it is run: Shared archive loaded. Link done. Shouldn't this sort of stuff only be output when debugging Scilab or in some sort of verbose mode? Lastly for the moment, I think you should add all of the Scilab contributor names into the COPYRIGHT file. More review comments to follow... I've also given you Github permissions to commit directly to the repository, so if okay with Sylvestre and Vincent, you can commit directly to the gsoc2012-scilab branch. William |
From: Sylvestre L. <syl...@sc...> - 2013-09-05 08:35:13
|
> Lastly for the moment, I think you should add all of the Scilab > contributor names into the COPYRIGHT file. More review comments to > follow... > Sure. > I've also given you Github permissions to commit directly to the > repository, so if okay with Sylvestre and Vincent, you can commit > directly to the gsoc2012-scilab branch. > Of course :) Sylvestre |
From: William S F. <ws...@fu...> - 2013-09-06 06:28:38
|
On 05/09/13 08:55, William S Fulton wrote: > On 04/09/13 17:38, Simon Marchetto wrote: >> Le 03/09/2013 19:18, Simon Marchetto a écrit : >>> >>> The error message I had on parallel make was something like "read >>> processus pipe: no such file". I couldn't figure out what was going on >>> exactly. >>> >>> But this is past : the test-suite can run now successfully in parallel >>> :-) >>> >>> For this I forced the Scilab make to work in serial. >>> After that, I had to parallelize test-suite build & run. There was >>> indeed conflicts between concurrent tests, because they build & run on >>> the same folder, using same files. > Is SWIG generating the same output file names? It should not, so if that > is the problem, this needs fixing on the SWIG side. If concurrent > instances of Scilab can't read the same input files, then I would > organise to get Scilab fixed as more than one instance of the same tool > should be able run concurrently if the outputs are different. > >>> Now each test builds in its own >>> subdirectory. >>> >>> The commit will come soon. I am impatient to test it on Travis. >>> >>> Regards, >>> >>> Simon >>> >> >> Hello William, >> >> Done ! >> As you probably have seen, the test-suite runs now in less than 20 >> minutes, with "-j4" :-) > This is good progress, thanks. > > Re the Examples makefile changes, I would prefer it to be a lot more > like the other languages, because I end up having to maintain this stuff > and it is so much easier if they are consistent. Can you put INCLUDE > back to how it was? The scilab and scilab_cpp targets invoke scilab, > they should only be compiling code, not running anything. Also why have > the get_swig_scilab_args and get_output_dir macros? As you are using > separate directories for the test-suite, did you notice that Java and C# > take this approach and was there something wrong with the techniques > used in there? If you think the makefiles can be improved, I'd like it > as a patch across all languages - we havn't used $(eval $(call ...)) to > date, I'm still trying to work out what exactly it is doing. Makefile > macros are great obfuscators and generally I try to minimise their use > as readability is way more important. On a related note, while reviewing, I noticed that the output file handling was wrong when using -outdir and probably also -outcurrentdir. I've committed a correction for this. It may break the test-suite, I'm not sure, as I only have scilab-5.2.1 on the machine I'm on today to check. As the output file handling is now consistent with other languages, it should be base the test-suite on the test-suite path handling from Java/C#. Note that this works using out of source builds (when using relative paths, such as './autogen.sh && mkdir build && cd build && ../configure'). I've also committed a couple of other minor improvements in the documentation and command line handling. One question, does -addsrc work for files with spaces in their names? If not I suggest using comma separated file lists as using a comma in a filename is a lot less common than a space. William |
From: Simon M. <sim...@sc...> - 2013-09-06 10:48:21
|
Le 05/09/2013 09:55, William S Fulton a écrit : > Is SWIG generating the same output file names? It should not, so if > that is the problem, this needs fixing on the SWIG side. If concurrent > instances of Scilab can't read the same input files, then I would > organise to get Scilab fixed as more than one instance of the same > tool should be able run concurrently if the outputs are different. > Yes. As you saw it, Swig generates a same file for all tests 'builder.sce', script that is executed by Scilab to build the test library. That could be changed. But there is another problem after: internally, Scilab will generate another file 'loader.sce', to load the test library. It is fixed, I've no way to change it ! So I decided to build each test in a separate directory, which seems to be simple & safe to me : more difficult to introduce a conflict in the future, it is also easier to clean the test, you delete the directory. I will take your remark and have a look as how Java and C# did that. > > Re the Examples makefile changes, I would prefer it to be a lot more > like the other languages, because I end up having to maintain this > stuff and it is so much easier if they are consistent. Can you put > INCLUDE back to how it was? The scilab and scilab_cpp targets invoke > scilab, they should only be compiling code, not running anything. Also > why have the get_swig_scilab_args and get_output_dir macros? As you > are using separate directories for the test-suite, did you notice that > Java and C# take this approach and was there something wrong with the > techniques used in there? If you think the makefiles can be improved, > I'd like it as a patch across all languages - we havn't used $(eval > $(call ...)) to date, I'm still trying to work out what exactly it is > doing. Makefile macros are great obfuscators and generally I try to > minimise their use as readability is way more important. > I understand the importance of consistency. What do you mean by "put INCLUDE back" ? 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 ? > The constants example needed fixing and didn't exit with a bad exit code: > > SWIG/Scilab Error : RuntimeError > Contract violation: require: (arg1>=0)&&(arg2>=0)SWIG/Scilab Error : > RuntimeError > Contract violation: require: (arg1>=0)checking Examples/scilab/enum > Bad exit code ? I will have a look at this. > I havn't checked yet your latest patches, but if this isn't sorted, > please add to your TODO list. > > Examples/scilab/enum => for consistency, can you remove > scilabconst_example.h and put the code in it into the example.i or > example.h file. Also use example.cxx not example.cpp. One day I would > like to create a common example framework. You probably have enough or > even too many examples. I'd prefer you to test any further changes > using the test-suite as the examples aren't very good for regression > testing. > I agree, let's make examples more consistent. Some of my example indeed look like more to tests than examples. I didn't understand fully the logic of example, tests, and their makefiles. And I thought also it was important to show the different possibilities in examples, as people read less often the unit tests or even sometimes the documentation ! But OK, I will rework this. > Could you arrange to have Scilab patched for the next version to get > rid of the completely pointless crud it is output each time it is run: > > Shared archive loaded. > Link done. > Done. By the way, thanks for your help and changes. Regards, Simon Marchetto |
From: William S F. <ws...@fu...> - 2013-09-06 17:52:06
|
On 06/09/13 11:47, Simon Marchetto wrote: > Le 05/09/2013 09:55, William S Fulton a écrit : >> Is SWIG generating the same output file names? It should not, so if >> that is the problem, this needs fixing on the SWIG side. If concurrent >> instances of Scilab can't read the same input files, then I would >> organise to get Scilab fixed as more than one instance of the same >> tool should be able run concurrently if the outputs are different. >> > Yes. As you saw it, Swig generates a same file for all tests > 'builder.sce', script that is executed by Scilab to build the test > library. That could be changed. > But there is another problem after: internally, Scilab will generate > another file 'loader.sce', to load the test library. It is fixed, I've > no way to change it ! I'd file that as a bug in Scilab then. I'd expect that should two people want to run different scripts at the same time in the same directory that it would work. > So I decided to build each test in a separate directory, which seems to > be simple & safe to me : more difficult to introduce a conflict in the > future, it is also easier to clean the test, you delete the directory. Yup, nothing wrong with that as a workaround. > I will take your remark and have a look as how Java and C# did that. >> >> Re the Examples makefile changes, I would prefer it to be a lot more >> like the other languages, because I end up having to maintain this >> stuff and it is so much easier if they are consistent. Can you put >> INCLUDE back to how it was? The scilab and scilab_cpp targets invoke >> scilab, they should only be compiling code, not running anything. Also >> why have the get_swig_scilab_args and get_output_dir macros? As you >> are using separate directories for the test-suite, did you notice that >> Java and C# take this approach and was there something wrong with the >> techniques used in there? If you think the makefiles can be improved, >> I'd like it as a patch across all languages - we havn't used $(eval >> $(call ...)) to date, I'm still trying to work out what exactly it is >> doing. Makefile macros are great obfuscators and generally I try to >> minimise their use as readability is way more important. >> > > I understand the importance of consistency. What do you mean by "put > INCLUDE back" ? Oops, I meant TARGET in 7a81f55ac955672c05df95f8c6f352ff7b1e709b in Examples/scilab/*/Makefile. Sorry for confusion. > 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) > I agree, let's make examples more consistent. Some of my example indeed > look like more to tests than examples. I didn't understand fully the > logic of example, tests, and their makefiles. And I thought also it was > important to show the different possibilities in examples, as people > read less often the unit tests or even sometimes the documentation ! But > OK, I will rework this. > Thanks. >> Could you arrange to have Scilab patched for the next version to get >> rid of the completely pointless crud it is output each time it is run: >> >> Shared archive loaded. >> Link done. >> > Done. > Nice. > By the way, thanks for your help and changes. No probs. William |
From: William S F. <ws...@fu...> - 2013-09-06 17:57:32
|
On 06/09/13 11:47, Simon Marchetto wrote: > Le 05/09/2013 09:55, William S Fulton a écrit : >> Is SWIG generating the same output file names? It should not, so if >> that is the problem, this needs fixing on the SWIG side. If concurrent >> instances of Scilab can't read the same input files, then I would >> organise to get Scilab fixed as more than one instance of the same >> tool should be able run concurrently if the outputs are different. >> > Yes. As you saw it, Swig generates a same file for all tests > 'builder.sce', script that is executed by Scilab to build the test > library. That could be changed. > But there is another problem after: internally, Scilab will generate > another file 'loader.sce', to load the test library. It is fixed, I've > no way to change it ! I'd file that as a bug in Scilab then. I'd expect that should two people want to run different scripts at the same time in the same directory that it would work. > So I decided to build each test in a separate directory, which seems to > be simple & safe to me : more difficult to introduce a conflict in the > future, it is also easier to clean the test, you delete the directory. Yup, nothing wrong with that as a workaround. > I will take your remark and have a look as how Java and C# did that. >> >> Re the Examples makefile changes, I would prefer it to be a lot more >> like the other languages, because I end up having to maintain this >> stuff and it is so much easier if they are consistent. Can you put >> INCLUDE back to how it was? The scilab and scilab_cpp targets invoke >> scilab, they should only be compiling code, not running anything. Also >> why have the get_swig_scilab_args and get_output_dir macros? As you >> are using separate directories for the test-suite, did you notice that >> Java and C# take this approach and was there something wrong with the >> techniques used in there? If you think the makefiles can be improved, >> I'd like it as a patch across all languages - we havn't used $(eval >> $(call ...)) to date, I'm still trying to work out what exactly it is >> doing. Makefile macros are great obfuscators and generally I try to >> minimise their use as readability is way more important. >> > > I understand the importance of consistency. What do you mean by "put > INCLUDE back" ? Oops, I meant TARGET in 7a81f55ac955672c05df95f8c6f352ff7b1e709b in Examples/scilab/*/Makefile. Sorry for confusion. > 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) > I agree, let's make examples more consistent. Some of my example indeed > look like more to tests than examples. I didn't understand fully the > logic of example, tests, and their makefiles. And I thought also it was > important to show the different possibilities in examples, as people > read less often the unit tests or even sometimes the documentation ! But > OK, I will rework this. > Thanks. >> Could you arrange to have Scilab patched for the next version to get >> rid of the completely pointless crud it is output each time it is run: >> >> Shared archive loaded. >> Link done. >> > Done. > Nice. > By the way, thanks for your help and changes. No probs. William |
From: William S F. <ws...@fu...> - 2013-09-17 23:06:18
|
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 |
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 |
From: William S F. <ws...@fu...> - 2013-10-03 06:35:27
|
On 01/10/13 09:24, Simon Marchetto wrote: > 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. > Why would vector<class> be supported and not vector<class*>? As far as the target language is concerned, these are nearly identical because SWIG presents a class and a pointer to to the language module as pretty much the same thing. In fact, vector<class*> can be more challenging because of memory management issues. > - 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). > It looks like Scilab can also handle different integer sizes and also boolean types, so this is more complex than just double and int. It looks like the primitive typemaps are not modelled on anything else, eg SWIG_AsVal_short silently casts from int to short. You are missing all the range checking - look at implementing the overload_numeric runtime tests. Python is usually a good reference, but it does separate out integral and floating point values, so a cast from a number that is a floating point number must be cast to an integral, eg: short smethod(short d); // C code # Python code print smethod(int(22.0)) # Okay print smethod(22.0) # Runtime error print smethod(22) # Okay Modelling on Octave ought to be a better fit where smethod(22.0) just works. Why do you want an option to add in casting like in Python? You could just make life easier for users by modelling it instead on the Octave module. Given the similarities between Octave, Matlab and Scilab, it would be a good thing to have them working similarly. William |
From: Simon M. <sim...@sc...> - 2013-10-09 16:32:23
|
Why would vector<class> be supported and not vector<class*>? As far as the target language is concerned, these are nearly identical because SWIG presents a class and a pointer to to the language module as pretty much the same thing. In fact, vector<class*> can be more challenging because of memory management issues. Indeed vector<class> will not be supported, but vector<class*> will. class and class* nearly identical from SWIG point of view ? But vector<class> contains copies of classes, while vector <class*> contains pointers to classes. So in the target language you could modify the content of the classes or not, depending on what you use. So they are not identical from target language point of view. But I probably misunderstand what you are telling me. It looks like Scilab can also handle different integer sizes and also boolean types, so this is more complex than just double and int. It looks like the primitive typemaps are not modelled on anything else, eg SWIG_AsVal_short silently casts from int to short. You are missing all the range checking - look at implementing the overload_numeric runtime tests. Python is usually a good reference, but it does separate out integral and floating point values, so a cast from a number that is a floating point number must be cast to an integral, eg: short smethod(short d); // C code # Python code print smethod(int(22.0)) # Okay print smethod(22.0) # Runtime error print smethod(22) # Okay Modelling on Octave ought to be a better fit where smethod(22.0) just works. Why do you want an option to add in casting like in Python? You could just make life easier for users by modelling it instead on the Octave module. Given the similarities between Octave, Matlab and Scilab, it would be a good thing to have them working similarly. Thanks for the advice, I will have a look on what is done in Octave, and surely base my work on it. Simon |
From: William S F. <ws...@fu...> - 2013-10-14 18:51:42
|
On 09/10/13 17:32, Simon Marchetto wrote: >> Why would vector<class> be supported and not vector<class*>? As far as >> the target language is concerned, these are nearly identical because >> SWIG presents a class and a pointer to to the language module as pretty >> much the same thing. In fact, vector<class*> can be more challenging >> because of memory management issues. > > Indeed vector<class> will not be supported, but vector<class*> will. > > class and class* nearly identical from SWIG point of view ? But > vector<class> contains copies of classes, while vector <class*> contains > pointers to classes. I see this as just a memory management issue. > So in the target language you could modify the content of the classes or > not, depending on what you use. So they are not identical from target > language point of view. Yeah, and neither is a vector of class or vector of class * the same in C++. They are very similar though, it is mostly the memory management that is different, but otherwise they are both containers of a particular class. Can you look at one of the other language modules and maybe you can explain in more detail why you can't implement something the same? William |
From: William S F. <ws...@fu...> - 2013-12-18 00:21:18
|
On 14/10/13 19:51, William S Fulton wrote: > On 09/10/13 17:32, Simon Marchetto wrote: >>> Why would vector<class> be supported and not vector<class*>? As far as >>> the target language is concerned, these are nearly identical because >>> SWIG presents a class and a pointer to to the language module as pretty >>> much the same thing. In fact, vector<class*> can be more challenging >>> because of memory management issues. >> >> Indeed vector<class> will not be supported, but vector<class*> will. >> >> class and class* nearly identical from SWIG point of view ? But >> vector<class> contains copies of classes, while vector <class*> contains >> pointers to classes. > I see this as just a memory management issue. > >> So in the target language you could modify the content of the classes or >> not, depending on what you use. So they are not identical from target >> language point of view. > Yeah, and neither is a vector of class or vector of class * the same in > C++. They are very similar though, it is mostly the memory management > that is different, but otherwise they are both containers of a > particular class. Can you look at one of the other language modules and > maybe you can explain in more detail why you can't implement something > the same? > What is the latest status for Scilab? Perhaps I've got it wrong, but not much development has happened recently and we are now in the final stages for putting out SWIG 3 and really I'd like Scilab support added for this release. William |
From: Simon M. <sim...@sc...> - 2013-12-20 08:30:53
|
Hello, Yes unfortunately I was away from SWIG. My aim is also to add Scilab in the major version SWIG 3. I am not sure if it is still possible now, if we want to keep high quality. My planned tasks were: - finish the C++ support: fix the vector<class> case, maybe a few small other things - fix primitive type typemaps, mostly about integer and double. I would like to introduce a SWIG feature that sets the behavior of that typemaps: either convert automatically int from/to Scilab double as Octave does, or keep integers as integers. - finish the documentation - check memory management - some refactoring : the generated code maybe or more organized or smaller (optional) - also, and what is the most important to me : I do not have a clear and complete view on what is supported or not by SWIG Scilab. I would like to build a check list of support status for all C or C++ features. For this, I need to analyze also all the unit tests, and maybe I need to add some tests too. Each of these task may be not so long to complete, but the sum could take a couple of weeks. Simon Le 18/12/2013 01:21, William S Fulton a écrit : > What is the latest status for Scilab? Perhaps I've got it wrong, but > not much development has happened recently and we are now in the final > stages for putting out SWIG 3 and really I'd like Scilab support added > for this release. > > William > |
From: William S F. <ws...@fu...> - 2013-12-20 20:57:15
|
I don't think you need go overboard on the tests for acceptance into master. So long as the test-suite passes and you have a few runtime tests (you already have enough), then I'll merge it into master. You can add improvements over time. All I think you need to do is get the primitive typemaps working as these are fundamental. With regard to checklists for C/C++ features supported, if the test-suite compiles and your tests include things like enums, global functions and variables, static and non-static class methods and variables, you'll find that you'll have a lot covered as a lot of the detail is handled in the core. Adding in stl wrappers etc can come later. William On 20/12/13 08:30, Simon Marchetto wrote: > Hello, > > Yes unfortunately I was away from SWIG. My aim is also to add Scilab in > the major version SWIG 3. > I am not sure if it is still possible now, if we want to keep high quality. > > My planned tasks were: > - finish the C++ support: fix the vector<class> case, maybe a few small > other things > - fix primitive type typemaps, mostly about integer and double. I would > like to introduce a SWIG feature that sets the behavior of that > typemaps: either convert automatically int from/to Scilab double as > Octave does, or keep integers as integers. > - finish the documentation > - check memory management > - some refactoring : the generated code maybe or more organized or > smaller (optional) > - also, and what is the most important to me : I do not have a clear > and complete view on what is supported or not by SWIG Scilab. I would > like to build a check list of support status for all C or C++ features. > For this, I need to analyze also all the unit tests, and maybe I need to > add some tests too. > > Each of these task may be not so long to complete, but the sum could > take a couple of weeks. > > Simon > > > Le 18/12/2013 01:21, William S Fulton a écrit : >> What is the latest status for Scilab? Perhaps I've got it wrong, but >> not much development has happened recently and we are now in the final >> stages for putting out SWIG 3 and really I'd like Scilab support added >> for this release. >> >> William >> > > |
From: Simon M. <sim...@sc...> - 2014-01-29 17:25:19
|
Hello William, I am back on SWIG ! I During the time I have been away, I could think about the initial tasks planned below. I decided to revise some of them: - We dont need a SWIG feature to set the behavior for primitive typemaps (conversion to/from double or not). The Scilab (signed and unsigned) integers will be used only if the wrapped function signature specifies signed or unsigned integers. In other cases, we convert from/to Scilab double, if possible. It is simpler. By the way that was the initial behavior, but it still needs to be fixed. The code of Scilab typemaps still need some cleaning in general. - Code generation : I wanted to optimize the generated code, to reduce its size, and increase its performance. But it can wait the next version. - Review of C/C++ features support. It will take too much time. I will review the most important unit tests, fix them and add a few if needed. I will do a complete review later. - STL support : I will fix only the case of vector<class> (we'll have to discuss about it) and a few other things (I need to review and probably clean the support of vector<enum>). I let away other features of STL: iterators... But I keep the task of finishing the documentation, which is one of the most important tasks. Also a generated code is not usable if it has huge memory leaks. I do not think it will have any, but if I have time, I will do some checks on memory management. I hope I can finish all this soon. Regards, Simon Le 20/12/2013 21:26, William S Fulton a écrit : > I don't think you need go overboard on the tests for acceptance into > master. So long as the test-suite passes and you have a few runtime > tests (you already have enough), then I'll merge it into master. You > can add improvements over time. All I think you need to do is get the > primitive typemaps working as these are fundamental. > > With regard to checklists for C/C++ features supported, if the > test-suite compiles and your tests include things like enums, global > functions and variables, static and non-static class methods and > variables, you'll find that you'll have a lot covered as a lot of the > detail is handled in the core. Adding in stl wrappers etc can come later. > > William > > On 20/12/13 08:30, Simon Marchetto wrote: >> Hello, >> >> Yes unfortunately I was away from SWIG. My aim is also to add Scilab in >> the major version SWIG 3. >> I am not sure if it is still possible now, if we want to keep high >> quality. >> >> My planned tasks were: >> - finish the C++ support: fix the vector<class> case, maybe a few small >> other things >> - fix primitive type typemaps, mostly about integer and double. I would >> like to introduce a SWIG feature that sets the behavior of that >> typemaps: either convert automatically int from/to Scilab double as >> Octave does, or keep integers as integers. >> - finish the documentation >> - check memory management >> - some refactoring : the generated code maybe or more organized or >> smaller (optional) >> - also, and what is the most important to me : I do not have a clear >> and complete view on what is supported or not by SWIG Scilab. I would >> like to build a check list of support status for all C or C++ features. >> For this, I need to analyze also all the unit tests, and maybe I need to >> add some tests too. >> >> Each of these task may be not so long to complete, but the sum could >> take a couple of weeks. >> >> Simon >> >> >> Le 18/12/2013 01:21, William S Fulton a écrit : >>> What is the latest status for Scilab? Perhaps I've got it wrong, but >>> not much development has happened recently and we are now in the final >>> stages for putting out SWIG 3 and really I'd like Scilab support added >>> for this release. >>> >>> William >>> >> >> > |
From: William S F. <ws...@fu...> - 2014-01-29 22:49:05
|
Simon, Your plan sounds reasonable. Definitely any fine tuning such as memory leak fixes can be done in later releases. Indeed the basic typemaps for primitives should work correctly. Also basic STL like std::vector should work reasonably well, at the minimum generating compilable code even if they cannot be usefully used from Scilab. In summary the test-suite should pass if these basics are all ready. SWIG is very forgiving and can easily generate compilable code even if all types are practically unusable and this is the minimum accepted standard. From what I recall of the Scilab test-suite you were practically there until something went wrong in the primitive typemaps and vectors. What precisely do you mean by the Scilab integers will only be used if the wrapped function specified signed or unsigned integers. All integers in C are either signed or unsigned, they can't be anything else! If you are thinking that 'signed' or 'signed int' should behave differently to 'int', then I will contest that. Surely you mean all integer types will convert to an equivalent Scilab integer type without exception and the only 'other cases' are floating point types, which of couse should map to a Scilab double? What about bool? William On 29 January 2014 17:09, Simon Marchetto < sim...@sc...> wrote: > Hello William, > > I am back on SWIG ! I During the time I have been away, I could think > about the initial tasks planned below. I decided to revise some of them: > > - We dont need a SWIG feature to set the behavior for primitive typemaps > (conversion to/from double or not). The Scilab (signed and unsigned) > integers will be used only if the wrapped function signature specifies > signed or unsigned integers. In other cases, we convert from/to Scilab > double, if possible. It is simpler. By the way that was the initial > behavior, but it still needs to be fixed. The code of Scilab typemaps still > need some cleaning in general. > > - Code generation : I wanted to optimize the generated code, to reduce its > size, and increase its performance. But it can wait the next version. > > - Review of C/C++ features support. It will take too much time. I will > review the most important unit tests, fix them and add a few if needed. I > will do a complete review later. > > - STL support : I will fix only the case of vector<class> (we'll have to > discuss about it) and a few other things (I need to review and probably > clean the support of vector<enum>). I let away other features of STL: > iterators... > > But I keep the task of finishing the documentation, which is one of the > most important tasks. > > Also a generated code is not usable if it has huge memory leaks. I do not > think it will have any, but if I have time, I will do some checks on memory > management. > > I hope I can finish all this soon. > > Regards, > > Simon > > > Le 20/12/2013 21:26, William S Fulton a écrit : > > I don't think you need go overboard on the tests for acceptance into >> master. So long as the test-suite passes and you have a few runtime tests >> (you already have enough), then I'll merge it into master. You can add >> improvements over time. All I think you need to do is get the primitive >> typemaps working as these are fundamental. >> >> With regard to checklists for C/C++ features supported, if the test-suite >> compiles and your tests include things like enums, global functions and >> variables, static and non-static class methods and variables, you'll find >> that you'll have a lot covered as a lot of the detail is handled in the >> core. Adding in stl wrappers etc can come later. >> >> William >> >> On 20/12/13 08:30, Simon Marchetto wrote: >> >>> Hello, >>> >>> Yes unfortunately I was away from SWIG. My aim is also to add Scilab in >>> the major version SWIG 3. >>> I am not sure if it is still possible now, if we want to keep high >>> quality. >>> >>> My planned tasks were: >>> - finish the C++ support: fix the vector<class> case, maybe a few small >>> other things >>> - fix primitive type typemaps, mostly about integer and double. I would >>> like to introduce a SWIG feature that sets the behavior of that >>> typemaps: either convert automatically int from/to Scilab double as >>> Octave does, or keep integers as integers. >>> - finish the documentation >>> - check memory management >>> - some refactoring : the generated code maybe or more organized or >>> smaller (optional) >>> - also, and what is the most important to me : I do not have a clear >>> and complete view on what is supported or not by SWIG Scilab. I would >>> like to build a check list of support status for all C or C++ features. >>> For this, I need to analyze also all the unit tests, and maybe I need to >>> add some tests too. >>> >>> Each of these task may be not so long to complete, but the sum could >>> take a couple of weeks. >>> >>> Simon >>> >>> >>> Le 18/12/2013 01:21, William S Fulton a écrit : >>> >>>> What is the latest status for Scilab? Perhaps I've got it wrong, but >>>> not much development has happened recently and we are now in the final >>>> stages for putting out SWIG 3 and really I'd like Scilab support added >>>> for this release. >>>> >>>> William >>>> >>>> >>> >>> >> > |
From: Simon M. <sim...@sc...> - 2014-01-30 09:47:11
|
OK, I agree. So are we going on the following behavior ? - Scilab double or integer in input => C int - C int in output => Scilab double - C unsigned int <=> Scilab unsigned integer (same for short, and long). It is the behavior you proposed last time, and which is currently used in Octave, if I remembered what you told me ? Maybe there are some use cases in which we want absolutely a Scilab signed integer returned, but in fact I can't see anyone for now. We'll see on using. About bool: there is a Scilab boolean type, mapping should be direct, no other conversion. Simon Le 29/01/2014 23:40, William S Fulton a écrit : > Simon, > > Your plan sounds reasonable. Definitely any fine tuning such as memory > leak fixes can be done in later releases. Indeed the basic typemaps > for primitives should work correctly. Also basic STL like std::vector > should work reasonably well, at the minimum generating compilable code > even if they cannot be usefully used from Scilab. In summary the > test-suite should pass if these basics are all ready. SWIG is very > forgiving and can easily generate compilable code even if all types > are practically unusable and this is the minimum accepted standard. > From what I recall of the Scilab test-suite you were practically there > until something went wrong in the primitive typemaps and vectors. > > What precisely do you mean by the Scilab integers will only be used if > the wrapped function specified signed or unsigned integers. All > integers in C are either signed or unsigned, they can't be anything > else! If you are thinking that 'signed' or 'signed int' should behave > differently to 'int', then I will contest that. Surely you mean all > integer types will convert to an equivalent Scilab integer type > without exception and the only 'other cases' are floating point types, > which of couse should map to a Scilab double? What about bool? > > William > > > On 29 January 2014 17:09, Simon Marchetto > <sim...@sc... > <mailto:sim...@sc...>> wrote: > > Hello William, > > I am back on SWIG ! I During the time I have been away, I could > think about the initial tasks planned below. I decided to revise > some of them: > > - We dont need a SWIG feature to set the behavior for primitive > typemaps (conversion to/from double or not). The Scilab (signed > and unsigned) integers will be used only if the wrapped function > signature specifies signed or unsigned integers. In other cases, > we convert from/to Scilab double, if possible. It is simpler. By > the way that was the initial behavior, but it still needs to be > fixed. The code of Scilab typemaps still need some cleaning in > general. > > - Code generation : I wanted to optimize the generated code, to > reduce its size, and increase its performance. But it can wait the > next version. > > - Review of C/C++ features support. It will take too much time. I > will review the most important unit tests, fix them and add a few > if needed. I will do a complete review later. > > - STL support : I will fix only the case of vector<class> (we'll > have to discuss about it) and a few other things (I need to review > and probably clean the support of vector<enum>). I let away other > features of STL: iterators... > > But I keep the task of finishing the documentation, which is one > of the most important tasks. > > Also a generated code is not usable if it has huge memory leaks. I > do not think it will have any, but if I have time, I will do some > checks on memory management. > > I hope I can finish all this soon. > > Regards, > > Simon > > > Le 20/12/2013 21:26, William S Fulton a écrit : > > I don't think you need go overboard on the tests for > acceptance into master. So long as the test-suite passes and > you have a few runtime tests (you already have enough), then > I'll merge it into master. You can add improvements over time. > All I think you need to do is get the primitive typemaps > working as these are fundamental. > > With regard to checklists for C/C++ features supported, if the > test-suite compiles and your tests include things like enums, > global functions and variables, static and non-static class > methods and variables, you'll find that you'll have a lot > covered as a lot of the detail is handled in the core. Adding > in stl wrappers etc can come later. > > William > > On 20/12/13 08:30, Simon Marchetto wrote: > > Hello, > > Yes unfortunately I was away from SWIG. My aim is also to > add Scilab in > the major version SWIG 3. > I am not sure if it is still possible now, if we want to > keep high quality. > > My planned tasks were: > - finish the C++ support: fix the vector<class> case, > maybe a few small > other things > - fix primitive type typemaps, mostly about integer and > double. I would > like to introduce a SWIG feature that sets the behavior of > that > typemaps: either convert automatically int from/to Scilab > double as > Octave does, or keep integers as integers. > - finish the documentation > - check memory management > - some refactoring : the generated code maybe or more > organized or > smaller (optional) > - also, and what is the most important to me : I do not > have a clear > and complete view on what is supported or not by SWIG > Scilab. I would > like to build a check list of support status for all C or > C++ features. > For this, I need to analyze also all the unit tests, and > maybe I need to > add some tests too. > > Each of these task may be not so long to complete, but the > sum could > take a couple of weeks. > > Simon > > > Le 18/12/2013 01:21, William S Fulton a écrit : > > What is the latest status for Scilab? Perhaps I've got > it wrong, but > not much development has happened recently and we are > now in the final > stages for putting out SWIG 3 and really I'd like > Scilab support added > for this release. > > William > > > > > > |
From: William S F. <ws...@fu...> - 2014-02-02 19:58:58
|
Simon This isn't clear to me what you will be doing. I think you need to write a more precise specification. It seems to me that for a given C numeric type as input, the marshalling should try its hardest to convert the Scilab type (be it a boolean, signed or unsigned integer type or double) into the given C numeric type. If it can't then it should give some sort of overflow error. With regard to output types, then if the C type is a signed integer type it should be converted into a signed Scilab integer. I don't see why you want to convert a C int in output to a Scilab double, you should use the closest match in size and sign to the C integer type. I suggest you next make a complete table of C types and Scilab types that you will convert to and from. This should form part of the documentation too. William On 30/01/14 09:47, Simon Marchetto wrote: > OK, I agree. > > So are we going on the following behavior ? > - Scilab double or integer in input => C int > - C int in output => Scilab double > - C unsigned int <=> Scilab unsigned integer > > (same for short, and long). > > It is the behavior you proposed last time, and which is currently used > in Octave, if I remembered what you told me ? > > Maybe there are some use cases in which we want absolutely a Scilab > signed integer returned, but in fact I can't see anyone for now. > We'll see on using. > > About bool: there is a Scilab boolean type, mapping should be direct, no > other conversion. > > Simon > > Le 29/01/2014 23:40, William S Fulton a écrit : >> Simon, >> >> Your plan sounds reasonable. Definitely any fine tuning such as memory >> leak fixes can be done in later releases. Indeed the basic typemaps >> for primitives should work correctly. Also basic STL like std::vector >> should work reasonably well, at the minimum generating compilable code >> even if they cannot be usefully used from Scilab. In summary the >> test-suite should pass if these basics are all ready. SWIG is very >> forgiving and can easily generate compilable code even if all types >> are practically unusable and this is the minimum accepted standard. >> From what I recall of the Scilab test-suite you were practically there >> until something went wrong in the primitive typemaps and vectors. >> >> What precisely do you mean by the Scilab integers will only be used if >> the wrapped function specified signed or unsigned integers. All >> integers in C are either signed or unsigned, they can't be anything >> else! If you are thinking that 'signed' or 'signed int' should behave >> differently to 'int', then I will contest that. Surely you mean all >> integer types will convert to an equivalent Scilab integer type >> without exception and the only 'other cases' are floating point types, >> which of couse should map to a Scilab double? What about bool? >> >> William >> >> >> On 29 January 2014 17:09, Simon Marchetto >> <sim...@sc... >> <mailto:sim...@sc...>> wrote: >> >> Hello William, >> >> I am back on SWIG ! I During the time I have been away, I could >> think about the initial tasks planned below. I decided to revise >> some of them: >> >> - We dont need a SWIG feature to set the behavior for primitive >> typemaps (conversion to/from double or not). The Scilab (signed >> and unsigned) integers will be used only if the wrapped function >> signature specifies signed or unsigned integers. In other cases, >> we convert from/to Scilab double, if possible. It is simpler. By >> the way that was the initial behavior, but it still needs to be >> fixed. The code of Scilab typemaps still need some cleaning in >> general. >> >> - Code generation : I wanted to optimize the generated code, to >> reduce its size, and increase its performance. But it can wait the >> next version. >> >> - Review of C/C++ features support. It will take too much time. I >> will review the most important unit tests, fix them and add a few >> if needed. I will do a complete review later. >> >> - STL support : I will fix only the case of vector<class> (we'll >> have to discuss about it) and a few other things (I need to review >> and probably clean the support of vector<enum>). I let away other >> features of STL: iterators... >> >> But I keep the task of finishing the documentation, which is one >> of the most important tasks. >> >> Also a generated code is not usable if it has huge memory leaks. I >> do not think it will have any, but if I have time, I will do some >> checks on memory management. >> >> I hope I can finish all this soon. >> >> Regards, >> >> Simon >> >> >> Le 20/12/2013 21:26, William S Fulton a écrit : >> >> I don't think you need go overboard on the tests for >> acceptance into master. So long as the test-suite passes and >> you have a few runtime tests (you already have enough), then >> I'll merge it into master. You can add improvements over time. >> All I think you need to do is get the primitive typemaps >> working as these are fundamental. >> >> With regard to checklists for C/C++ features supported, if the >> test-suite compiles and your tests include things like enums, >> global functions and variables, static and non-static class >> methods and variables, you'll find that you'll have a lot >> covered as a lot of the detail is handled in the core. Adding >> in stl wrappers etc can come later. >> >> William >> >> On 20/12/13 08:30, Simon Marchetto wrote: >> >> Hello, >> >> Yes unfortunately I was away from SWIG. My aim is also to >> add Scilab in >> the major version SWIG 3. >> I am not sure if it is still possible now, if we want to >> keep high quality. >> >> My planned tasks were: >> - finish the C++ support: fix the vector<class> case, >> maybe a few small >> other things >> - fix primitive type typemaps, mostly about integer and >> double. I would >> like to introduce a SWIG feature that sets the behavior of >> that >> typemaps: either convert automatically int from/to Scilab >> double as >> Octave does, or keep integers as integers. >> - finish the documentation >> - check memory management >> - some refactoring : the generated code maybe or more >> organized or >> smaller (optional) >> - also, and what is the most important to me : I do not >> have a clear >> and complete view on what is supported or not by SWIG >> Scilab. I would >> like to build a check list of support status for all C or >> C++ features. >> For this, I need to analyze also all the unit tests, and >> maybe I need to >> add some tests too. >> >> Each of these task may be not so long to complete, but the >> sum could >> take a couple of weeks. >> >> Simon >> >> >> Le 18/12/2013 01:21, William S Fulton a écrit : >> >> What is the latest status for Scilab? Perhaps I've got >> it wrong, but >> not much development has happened recently and we are >> now in the final >> stages for putting out SWIG 3 and really I'd like >> Scilab support added >> for this release. >> >> William >> >> >> >> >> >> > |