Updates to lua bindings, part 1. #104

Merged
merged 47 commits into from Feb 28, 2014

Conversation

Projects
None yet
2 participants
Contributor

v-for-vandal commented Nov 11, 2013

Hi @wsfulton!

This is first part of updated lua bindings.
Main changes:

  1. Class inheritance. Now, instead of merging functions/constants etc from all class bases into its table, class maintains a lua array with SWIG names of its bases. Getter/setter then traverse this list in depth-first manner.
  2. Code refactoring. The main structure was retained, but some code was moved into separate functions and process of generating C-arrays with names of all functions/getters/setters etc was improved. IMHO it is now more understandable and maintainable.
  3. Namespaces now fully supported. Each namespace is treated like a module. Module is just a table in lua. Global namespace becomes main module table, all other namespaces - subtable.
  4. Changes to lang.cxx: original implementations of membervariableHandler set an attribute(flag) "memberget"/"memberset" before calling functionWrapper. I added same thing to variableWrapper, flags are called "varget"/"varset".

Drawbacks:
There were (and still are) options (-elua/-eluac) that make SWIG generate methods/functions arrays in a way that was compatible with embeded lua. The code for this was taken directly from current bindings. Unfortunatelly I have failed to test them. Couldn't find any way to build and setup elua for usual PC. I will try to tackle this problem, but I would like to know your opinion on whether it is acceptable to mark elua support as experimental/use-on-your-own-risk-and-send-us-patches. All I can guarantee now is that elua bindings compile.
On the other hand, I have improved elua bindings and in theory they should now pass usual tests.

Compatibility:
New bindings maintains compatibility with previous version - old names are generated by default(verified by tests) A new cmd option was introduced that allows user to enable/disable compatibility.

Tests:
all tests are ok.

Current state:
All changes that were planned for this pull request were implemented. There is some debugging code inside, it is marked with comments '// TODO:REMOVE' in case some further changes will be necessary. And I didn't made any changes to documentation.

If the idea of the patch is ok with you:

  1. I am ready to fix any issues that may arise and feedback is always welcome
  2. If you give me preliminary approvement of request, I will start updating documentation.

Patch acts as prerequisite to:

  1. Moving to standard typemaps library. (WIP)
  2. Directors support. (WIP)
Owner

wsfulton commented Nov 12, 2013

Thumbs up for preliminary approval. Some comments/queries etc follow...

'namespace now supported' Lua namespace/C++ namespace or %nspace?

printNamespaceDefinition ? Lua namespace or C++ namespace

I don't think I've quite understood all the %nspace implementation. I'm waiting for your documentation about
this to understand fully. The implementation is a bit confusing for me as I think the code implementation
using 'namespace' for a Lua namespace without actually saying so. In SWIG I have been using the term nspace for
something akin to a target language namespace (such as a package in Java). http://www.lua.org/pil/15.html suggests there is no such thing as a namespace in Lua and instead use the term 'package' and represent packages with tables. How do your Lua namespaces relate to what Lua calls packages in the Lua docs?

What with the confusing terminology and not looking in too much detail and the lack of explaining what the namespace hash is for, I don't really get the point of getNamespaceHash. It might just be me, but it seems like it is replicating everything already in the symbol tables.

embedded Lua - Can you contact Raman Gopalan who contributed the embedded patches and get him involved in testing, otherwise it will have to go experimental as you suggest.

Which versions of Lua are you testing with (examples and test-suite)? I only seem to have 5.1.x wherever I look.

Can you use camelCase style for member functions in LUA, eg symname_wrapper => symnameWrapper. SWIG isn't very consistent in this area, but that is what it is mostly like.

Please use Swig/SWIG/swig as prefix instead of _swig in the runtime code.
C++ comments in the runtime code must be changed to C comments in order to work with all C compilers.
Swig_typemap_lookup should use typemap warnings not error

Is there anything in the Java/C# nspace implementation that you re-used and think could be abstracted out?

Can you change your new function comments (and any others in Lua) to fit in with SWIG style, something like this:

/* ---------------------------------------------------------------------

  • function_name()
    *
  • Something interesting about this function...
  • --------------------------------------------------------------------- */

Checks like this

  • assert(ns_methods_tab != 0);
    are normally this sort of style:
  • assert(ns_methods_tab);

When you do the documentation, please put in a section about backwards compatibility. This should be quite detailed to help upgrade.
I also need a user level summary of changes and incompatibilities for the CHANGES.current file.

I'm not so keen on having the -api-lvl-from option as it kind of encourages non-backwards compatibililty as a norm, when it certainly is not the norm. The other language modules deal with this by using names for new features. So in your case where the default is no change, add a new named option for the SWIG 3 api. This also means that additional changes can be added without relying on SWIG upping the version number and names are more searchable/identifiable/understandable than numbers. Perhaps at the end of your changes, the api might never change and we decide to make the new api the norm and users have to add in a -swig2 kind of switch to get the older code generation. Let's see how you get on.

Contributor

v-for-vandal commented Nov 13, 2013

Ok. Part 1 - lets make things clear with namespace.
TL;DR; It is support of %nspace feature. It partially corresponds to lua package. It is a lua table, as package is, but you cant call require() or import() on it. It is embeded in the module itself.

Now, lets forget about SWIG for a moment. Let's say we have only Lua and C++ and let's look at the example from mentioned link http://www.lua.org/pil/15.html. Package "complex":

    complex = {}

    function complex.new (r, i) return {r=r, i=i} end

 -- defines a constant `i'
    complex.i = complex.new(0, 1)

    function complex.add (c1, c2)
      return complex.new(c1.r + c2.r, c1.i + c2.i)
    end

In the end, this lua code will result in just a lua table that holds as members:

  1. one constant (i)
  2. a few functions (new, add, etc.)
    All this code, described in chapter 15.1 - 15.5 is here only to deal with Lua name resolution scheme, table accessing scheme and all other lua syntax and semantics rules.

The logical equivalent in terms of C++ namespaces would look like this (lets ignore some C++ naming rules for consistency):

namespace complex {
    struct Complex {
        int r;
        int i;
    }
    // constant
    const Complex i = {0,11}; 
    // functions
    Complex new(int,int);
    Complex add( const Complex&, const Complex&);
}

Now, lets add one more C++ namespace:

namespace math {
namespace complex {
    struct Complex {
        int r;
        int i;
    }
    // constant
    const Complex i = {0,11}; 
    // functions
    Complex new(int,int);
    Complex add( const Complex&, const Complex&);
}
}

Lets look at the logical equivalent in Lua. We want consistency in corresponding names in lua, so at the end all we want is to have a 'math.complex.i' instead of 'complex.i', 'math.complex.add' instead of 'complex.add'. There are multiple ways to do this, for example this wil do:

   math = {}
    math.complex = {}

    function math.complex.new (r, i) return {r=r, i=i} end

 -- defines a constant `i'
    math.complex.i = math.complex.new(0, 1)

    function math.complex.add (c1, c2)
      return math.complex.new(c1.r + c2.r, c1.i + c2.i)
    end

or like this

   -- use previously defined complex package
   math = {}
   import("complex")
   math.complex = complex

or tens of other ways. But the result is always the same:
we have lua table named 'math' that contains lua table named 'complex' that in turn contains constants and methods etc.

Lets add more methods to math namespace:

namespace math {
double sin(double);
double cos(double);

namespace complex {
    struct Complex {
        int r;
        int i;
    }
    // constant
    const Complex i = {0,11}; 
    // functions
    Complex new(int,int);
    Complex add( const Complex&, const Complex&);
}
}

In lua it can be expressed, for example, like this(but this is not the only way to do it):

    math = {}
    function math.sin(x)
       -- calcualte y = sin(x) here
    return y
    end

    function math.cos(x) return y end

    math.complex = {}

    function math.complex.new (r, i) return {r=r, i=i} end

 -- defines a constant `i'
    math.complex.i = math.complex.new(0, 1)

    function math.complex.add (c1, c2)
      return math.complex.new(c1.r + c2.r, c1.i + c2.i)
    end

The result is as in previous example - table 'math' contains a few functions and a table 'complex' that contains .... etc.
So, whenever in C++ I would use math::complex::add in Lua I would write math.complex.add

So, summing up:
In lua package is represented by a table. The whole chapter 15 is about writing packages correctly in lua, but in the end it is a table. It can contain functions, numbers, etc and other tables. And those other tables could have come from other packages as well - no one bothers.

Now, whith %nspace enabled I map each C++ namespace to its own lua table. All methods/variables etc from this namespaces goes to this table. If it includes other namespace, then this other namespace gets its own lua table as well and this lua table is added to the parent namespace lua table as a member.
If you run SWIG over C++ example, then you will get the same structure as with lua example:
lua table 'math' that holds a few functions and a lua table 'complex' that in turns holds a few functions and a constant. The only difference is that everything was writen in C++, not in Lua.

Everything goes to single file. Its not like Java or C#, where each namespace is a separated directory etc. In Lua, all you can get from %nspace is proper separation of various functions to proper tables. Functions from 'math' namespace goes to table 'math', from 'complex' namespace - to table 'complex' and table 'complex' itself goes to the table 'math'. But everything reside in same '.so' object.

P.S. Technically, it is feasible to put every namespace into its own '.so' library. But in the end it will result in the same thing - separation of methods into proper tables, Plus extra complexities - managing loading/unloading multiple '.so' in correct order, some SWIG-related stuff that will definetly arise and so on. No benifits and a lot of drawbacks.

Contributor

v-for-vandal commented Nov 13, 2013

Part 2. getNamespaceHash and so on.
TL;DR: getNamespaceHash and other methods from this family are not about symbols, they are about storing C arrays that holds records about methods, constants, variables etc. before we could dump those arrays to wrappers.

Everything important in SWIG:Lua generated bindings is described by 3-5 C arrays: an array for methods, for getters, for setters, for constants etc. All arrays looked like:

static swig_lua_method _$classname_methods[] = {
method_record_1,
method_record_2,
....
}
static wig_lua_attributes _$classname_attributes[] = {
attribute_record_1,
attribute_record_2,
...
}

(Not only classes, but the global functions/variables are put in same arrays, only it is not $classname, but some other naming scheme).
Each array is held by String* variable.

In original implementation the were:

  1. one set of such arrays for global functions, global variables, global constants and all classes static functions, varibales etc. Lets call it global set
  2. one set of such arrays for every class. Lets call it class set(s)
    Class variables to hold global arrays were created inside Lua::top method, then all the parsing took place, then they were dumped to wrappers and later deleted.
    Class variable to hold class arrays were assigned new empty string for every class inside classHandler, then filled during class parsing and dumped to wrappers and deleted.

After my first patch (classobj-r2) the scheme became more complex: there were now 3 types of array sets:

  1. Global, without any changes.
  2. Set of arrays for class instance (non-stȧtic) members
  3. Set of arrays for class static members.
    As usual, global arrays were created at the begining, and arrays for class instance and static members were created for every class in the classHandler.
    Now, it was 3 sets x 5 arrays in each = 15 variables to manage manually at any point in time.
    Now, 2 considerations arised:
  4. Those array initialization was mostly the same for each set. E.g. it is always static swig_lua_method XXX[], for global methods array, for static class methods array and for class instance methods array. Only the XXX changes. Same for constants etc.
  5. With support for nspace it is now undetermined number of arrays sets: 2 sets (5 arrays in each) for every class, one set for every namespace + one set for module.

So I decided to move all code to initialize those arrays, to close them (close = add final bracket :) and printing them to file to separate functions. That is what getNamespaceHash, closeNamespaceHash and printNamespaceDefinition are about.

Now, namespaces provides additional issues:

  1. They aren't like classes, I dont get a namespace to parse and then parse all symbols in it and then I am done with this one and move to next namespace. It's more like symbol from NSpace1, then symbol from NSpace2, then again symbol from NSpace1. So, I needed to store arrays set that describes NSpace1 while I am parsing symbol from NSpace2 and get them back when I meet again symbol from NSpace1. That is why they should be stored permanently in namespace_hash untill all parsing is done.
  2. Emty namespaces that have no symbols, but have child namespaces. They still need those arrays, even if then only non-empty array is array with child namespaces. If I created arrays set for namespace only when I encountered symbol from this namespace, I would never generate those arrays for them. That is why there is a registration process and why arrays set is created on first request.. When namespace A.B.C.D is encountered, it requests A.B.C (it's parent) and adds itself to the appropriate C array. If record for A.B.C doesn't exist yet, it is created in the same fashion (thus creating A.B and A if necessary).

I admit, it is probably possible to use symbols table instead of 'namespace_hash' for storing this info. But other then having one Hash* variable less, there are no other benifits. 'namespace_hash' is more like a legacy from times when I wasn't sure what would be final structure of all this stuff. But it makes no harm and probably prevents from possible names clashes etc.

Contributor

v-for-vandal commented Nov 13, 2013

Part 3 - Various.

  1. I run tests with lua 5.1 and 5.2
    Examples wouldn't run with lua 5.2 because they use C-API that is only present in lua 5.1. I tried to fix them, but quickly gave in.
  2. I am sorry. I am not yet familiar with Swig_typemap_* family of functions and I cant understand where does this error "Swig_typemap_lookup should use typemap warnings not error" come from. Could you please point me to the function/line of code ?
Contributor

v-for-vandal commented Nov 13, 2013

CHANGES:

  1. %nspace support. Namespaces are mapped to tables in the module, with same name as namespace. Namespaces structure (namespace inside namespace inside namespace etc.) is preserved.
  2. Inheritance is now handled differently. Each class metatable keeps a list of class bases instead of merging all members of all bases into the derived class. (comment: This is NOT disabled in backward-compatible mode)
  3. Special class metatable member ".constructor" was removed. Now SWIG generated proxy function by itself and assigns it directly to class table "__call" method.
  4. (comment:untested) eLua should support inheritance too.
  5. (comment:untested) 'const' subtable in eLua considered deprecated. Created only in compatibility mode

INCOMPATIBILITIES:
0. You can no longer assign to unexisting class members in classes without __setitem method. It would cause a lua error.

  1. (only in swig3 mode) In C mode enums inside structure are now exported as in C++ - into structure table. They are not exported into module in any form, neither with prefixing $structname_$enumname, nor without $enumname. The latter variant is the behaviour of C standard.
  2. You can no longer iterate over module table and copy everything into global namespace. Actually, it was never the case, but it is time to set this explicitly so that nobody ever tried.
  3. Now changing base class will immideatelly affects all derived classes.
  4. There might be some issuse with inheritance. Although the bases iteration scheme is the same as was used for merging base classes into derived one, some unknown and uncovered by tests issues may arise.
Contributor

v-for-vandal commented Nov 13, 2013

I have updated (partially, still waiting for eLua resolution) documentation. I am not putting it here in order not to trigger Travis CI build on every doc update. Please look at it in my branch https://github.com/v-for-vandal/swig/blob/class_p1_doc/Doc/Manual/Lua.html

Contributor

v-for-vandal commented Nov 16, 2013

Hi @wsfulton
I have received no answer from Ramon Gopalan. I used this address: "ramangopalan@gmail.com" from swig-devel list. UPDATE: I have successfully contacted him and asked for help and review.
Meanwhile I have implemented eLua emulation mode and tested eLua with it. Seems working, all tests are passing except for one. That is due to difference in a way char constants are wrapped for Lua and for eLua. No way to fix this, though.
Can't test -eluac. It would never be able to pass tests. Can't even understand what are its use cases. But it compiles.

Are there any other things you would like me to fix ?

Owner

wsfulton commented Nov 23, 2013

@v-for-vandal Please confirm if this is ready now, including documentation in Lua.html. If so, I'll add it to the queue for review and merge... I got quite a few atm with SWIG 3 coming up! Some of the explanations of C++ namespaces and %nspace for Lua look like it would be useful in Lua.html.

Re "Swig_typemap_lookup should use typemap warnings not error" ...

+   tm_v2 = Swig_typemap_lookup("constcode", n_v2, name, 0);
+   if (!tm_v2) {
+     // This can't be.
+     assert(false);
+     Swig_restore(n);
+     return SWIG_ERROR;
+   }

Look in the other modules and you'll see that a warning is issued if the lookup fails, eg in python when constcode fails:

      Swig_warning(WARN_TYPEMAP_CONST_UNDEF, input_file, line_number, "Unsupported constant value.\n");
Contributor

v-for-vandal commented Nov 24, 2013

@wsfulton No, not yet ready. I am waiting for review/help from Ramon Gopalan, it should take a week.

The reason for this assert is that similar request for typemap was made 50 lines higher (same function). As I understand, the only way for one request made 2 times in one function to succeed first time and fail second one is some very nasty memory overwriting made by some rogue pointer. Thats why it is assert, not warning.

I can change it to the warning, e.g.

Swig_warning(WARN_TYPEMAP_CONST_UNDEF, input_file, line_number,
    "Ooops. Grave internal error. File a bug report please and disable backward compatibility for now.\n");
Owner

wsfulton commented Dec 23, 2013

We are just about ready for SWIG 3.0.0 and a lot longer than a week has gone by now! Is this realistically going to be ready in the coming days?

Contributor

v-for-vandal commented Dec 23, 2013

@wsfulton No, I doubt it. Lets merge it this way and I will handle eLua later. I am pretty sure it should work. I will create pull request for a documentation update branch in a few days. It won't contain any code so no intensive review/testing will be necessary.

Contributor

v-for-vandal commented Dec 23, 2013

I assumend you were asking about eLua specific part. Because I consider the general part completed and tested. There will be more pull requests from me in some not too distant future, but I have put into this pull request everything I wanted to.

Owner

wsfulton commented Dec 24, 2013

eLua specific fixes can come later.

I'd like the documentation (separate patch is fine) before pulling this one in. Partly because it will help me get a better grip on the changes, and secondly for users.

Owner

wsfulton commented Dec 24, 2013

With regard to the namespace_hash , this looks like a replication of the language symbol table. Can you incorporate it into the language symbol table? Take a look at -debug-lsymbols. Language::addSymbol() is the implementation.

Contributor

v-for-vandal commented Dec 26, 2013

@wsfulton Ok, I have removed namespace_hash and now symbol table is used. I also merged 'doc' branch. The only thing left is to remove all debugging code (marked as 'TODO:REMOVE'). As soon as Travis tests my pull request (see below) and you won't have further requests for substantial changes, I will do it and that will be all.

For some reason, Travis is not picking up my pull request and not testing it. Could you please look into it ? Is there something I should do ?

Owner

wsfulton commented Dec 26, 2013

There is a conflict merging this pull request to master, hence Travis cannot apply the patch to test it. You'll need to rebase it onto master.

Contributor

v-for-vandal commented Dec 27, 2013

@wsfulton Tests are ok. Any other changes you would like me to implement ?

Owner

wsfulton commented Feb 1, 2014

@v-for-vandal I must apologise for not responding earlier, I have been pretty busy and the size off the patch has put me off from getting to it! However, I would really like to move this forward asap as swig-3.0.0 is just about ready and this is the last big thing holding it up from being released. I promise to respond quickly from now on for this patch!

I have numerous comments. Have you got time to work on them this week?

I get numerous seg faults merging this into master such as the one below. Can you replicate?

checking testcase allowexcept under lua
Swig_save('lua_staticmembervariableHandler','cdecl'): Warning, attribute 'static' was already saved.
make[2]: *** [lua_cpp] Segmentation fault
make[1]: *** [allowexcept.cpptest] Error 2
Stacktrace:
(gdb) where
#0  Swig_save (ns=0x8181d84 "lua_staticmembervariableHandler", n=0xb7fb79e8) at Swig/tree.c:333
#1  0x080d0602 in LUA::staticmembervariableHandler (this=0x81ae6d0, n=0xb7fb79e8) at Modules/lua.cxx:1577
#2  0x080cc445 in Language::variableHandler (this=0x81ae6d0, n=0xb7fb79e8) at Modules/lang.cxx:1379
#3  0x080cd9e4 in Language::cDeclaration (this=0x81ae6d0, n=0xb7fb79e8) at Modules/lang.cxx:1046
#4  0x080cab90 in Dispatcher::emit_one (this=0x81ae6d0, n=0xb7fb79e8) at Modules/lang.cxx:117
#5  0x080caf94 in Language::emit_one (this=0x81ae6d0, n=0xb7fb79e8) at Modules/lang.cxx:397
#6  0x080cf4ba in Dispatcher::emit_children (this=0x81ae6d0, n=0xb7fb7788) at Modules/lang.cxx:209
#7  0x080c8781 in Language::classHandler (this=0x81ae6d0, n=0xb7fb7788) at Modules/lang.cxx:2500
#8  0x080d1a31 in LUA::classHandler (this=0x81ae6d0, n=0xb7fb7788) at Modules/lua.cxx:1273
#9  0x080c99e9 in Language::classDeclaration (this=0x81ae6d0, n=0xb7fb7788) at Modules/lang.cxx:2467
#10 0x080cac30 in Dispatcher::emit_one (this=0x81ae6d0, n=0xb7fb7788) at Modules/lang.cxx:125
#11 0x080caf94 in Language::emit_one (this=0x81ae6d0, n=0xb7fb7788) at Modules/lang.cxx:397
#12 0x080cf4ba in Dispatcher::emit_children (this=0x81ae6d0, n=0xb7fb7338) at Modules/lang.cxx:209
#13 0x080c6228 in Language::includeDirective (this=0x81ae6d0, n=0xb7fb7338) at Modules/lang.cxx:647
#14 0x080caec1 in Dispatcher::emit_one (this=0x81ae6d0, n=0xb7fb7338) at Modules/lang.cxx:163
#15 0x080caf94 in Language::emit_one (this=0x81ae6d0, n=0xb7fb7338) at Modules/lang.cxx:397
#16 0x080cf4ba in Dispatcher::emit_children (this=0x81ae6d0, n=0xb7fa8da8) at Modules/lang.cxx:209
#17 0x080d2af3 in LUA::top (this=0x81ae6d0, n=0xb7fa8da8) at Modules/lua.cxx:374
#18 0x080d91dc in SWIG_main (argc=6, argv=0xbffff664, l=0x81ae6d0) at Modules/main.cxx:1289
#19 0x08136604 in main (margc=6, margv=0xbffff664) at Modules/swigmain.cxx:206

There are some new options I would like you to remove/change:

  • the [NUM] addition to -elua and -eluac. Instead add in the #ifndef around MIN_OPT_LEVEL with the previous default. This fits in with the usual way to control code for non-major features... a user can use the generic -DMIN_OPT_LEVEL=x instead or put it into a %begin block.
  • -swig3. I don't want to have individual languages doing their own SWIG api version numbering. Sorry about this, I know I sort of suggested something like this earlier on. The norm is to use a command line switch to enable features. Can you instead think of a name for your changes and replace -swig3 with that name instead. Is there much more to do, I was expecting more to have arrived by now as this patch was titled part 1? I'm wondering if there will be breakages between 3.0.0 and later versions when using this option.

Please put the new options in alphabetical order.

Regarding lua:name and other static variables in lua.cxx that store the names for nspace support, I know this is bit of a mess, and will be cleaned up one day. Can you please make changes based on the nspace support in java.cxx/csharp.cxx (but reference the version from swig-2.0.11 prior to nested class additions)? It looks like class_symname should be called proxy_class_name, class_fq_symname should be full_proxy_class_name. This might seem pedantic, but given lua is the first scripting language to add in nspace support, I want to keep this as a reasonable template for the other scripting languages to use for nspace support. This is important given it also ties up with necessary nested class support for the future too.
Why do you have class_parent_nspace instead of calling getNSpace() directly? It should be called directly or is there some bug with it?
I think a lot of the above deviancy from the norm is because the Swig_name_xxx() functions are not being used. Can you please use them, that is, Swig_name_construct (for constructor), Swig_name_destroy (for destructors), Swig_name_get() Swig_name_set() for variable getters/setters and Swig_name_member(). I recommend looking at usage in Python. This should also mean the changes you made in the Language class are no longer needed. You then should not need mangled_class_fq_symname. should come from a call to Swig_name_construct. When it comes to supporting directors and nested classes, you'll find it much easier if Lua is using the usual support functions for names.

Contributor

v-for-vandal commented Feb 14, 2014

@wsfulton Hi. Sorry for the delay, I just got from the vacation with very little access to Internet. I will look into mentioned bugs and provide other fixes/explanations/renamings ASAP.

Owner

wsfulton commented Feb 19, 2014

hi @v-for-vandal, hope you had a good break. I've put out a beta of 3.0.0 and the last thing that must go into the final 3.0.0 is this set of patches. Is it just a matter of days before you can provide latest feedback?

v-for-vandal added some commits Oct 28, 2013

@v-for-vandal v-for-vandal Initial implementation - everything compiles but might not work 1c5a0f8
@v-for-vandal v-for-vandal nspace.i example is working 295788c
@v-for-vandal v-for-vandal Bugfixes 602cf3a
@v-for-vandal v-for-vandal Add runtime test aec4391
@v-for-vandal v-for-vandal Add pointer guard ad375d9
@v-for-vandal v-for-vandal Fixing issuse with v2-compatible static function names 577f20c
@v-for-vandal v-for-vandal More changes. Mostly to the would-be class library 63a26c6
@v-for-vandal v-for-vandal Preparations before pull request - part 1 aa1b829
@v-for-vandal v-for-vandal Bugfixes 7e052c1
@v-for-vandal v-for-vandal Bugfixes. CMD args handling. Code cleanup da05103
@v-for-vandal v-for-vandal Bugfixes 9d6cd75
@v-for-vandal v-for-vandal Fixes for elua c775e66
@v-for-vandal v-for-vandal Some class bases iteration improvements afd269f
@v-for-vandal v-for-vandal A few bugfixes dcbcac4
@v-for-vandal v-for-vandal Bugfixes f1fb2cc
@v-for-vandal v-for-vandal Removing obsolete debug code a3515ca
@v-for-vandal v-for-vandal Valuewrapper test 9bd39fe
@v-for-vandal v-for-vandal Code beautifier 1e282e3
@v-for-vandal v-for-vandal Manually beautifying luarun.swg eb7c0f0
@v-for-vandal v-for-vandal Remove some obsolete code 02c4a8e
@v-for-vandal v-for-vandal Remove some typos 7e09b66
@v-for-vandal v-for-vandal Fixes for examples. Wrapped keywords into guardian in keyword_rename …
…test
ce2760f
@v-for-vandal v-for-vandal Attempt to fix unreproducable bug (from Travis CI build) 89c6fbb
@v-for-vandal v-for-vandal Some fixes for elua a877102
@v-for-vandal v-for-vandal Style fixes. Comments fixes. Fixing cmd options. etc 4b0ed73
@v-for-vandal v-for-vandal Add support for C-style enums in C mode. And tests.
In backward compatible mode C style enums binding are correctly
generated
6d49a57
@v-for-vandal v-for-vandal Add compatibility option for old-style inheritance 0ee724c
@v-for-vandal v-for-vandal Bugfixes for eLua. eLua emulation mode 705beb6
@v-for-vandal v-for-vandal Small bugfixes 0c6263a
@v-for-vandal v-for-vandal Attempt to catch unreproducable bug from Travis CI build 14452ca
@v-for-vandal v-for-vandal Eliminating namespaces_hash and using symbols table instead e6d0f13
@v-for-vandal v-for-vandal Updating Lua documentation 3d36a69
@v-for-vandal v-for-vandal Small documenation fixes 00f59ac
@v-for-vandal v-for-vandal Rename methods to make it clear what 'symbols table' they operate on. 4eef510
@v-for-vandal v-for-vandal Fixes to module options 2767f2a
Contributor

v-for-vandal commented Feb 19, 2014

Fixed the cmd options. I have replaced -swig3 with -drop-old-scheme (name is discussable). Unfortunately, I can't find a way to get rid from underlying internal API versioning scheme ( variables api_level=X and corresponding to it vX_compatibility, currently X == 2). I can, of course rename v2_compatibility to something like "DoNotDropOldSchemeX"(X==2), but it doesn't seem different in any way.
The number (X) was not meant to be SWIG version but rather internal module API version. If I were more productive, it might as well be that for former option -swig3 the corresponding api_level would have been X=7 :)
As usual, everything is discussable and if you point me to a right way, I would gladly implement it.

Contributor

v-for-vandal commented Feb 19, 2014

  1. class_parent_nspace. Legacy, removed. Thanks.
  2. Swig_name_. Could you please clarify ? Do you mean that instead of remembering name of the generated C function as "memberget:wrap:name", "varset:wrap:name" etc I should use instead Swig_member_get etc ?
    I have tried it, but it causes a lot of troubles. I have to know how Language:: methods uses these Swig_name_
    methods and what arguments do they pass. Why, for example, Language::staticmemberfunctionHandler doesn't pass NSpace into
mrename = Swig_name_member(0, ClassPrefix, symname)

And if this implementation-dependent logic changes at some point, somebody would have to browse the lua module source code and make the same changes there.
With the way it is now, the corresponding C function name is generated only once (by Language::* methods) and remembered, and there is no guessing later what chain of Swig_name_* led to it's final name etc.

Again, with some effort I could overcome all those problems. I just don't think it is a good idea.

Owner

wsfulton commented Feb 19, 2014

The name is somewhat non-descript. How about something like -old-metatable-bindings instead of -drop-old-scheme. And even better give replace 'old' with something else more relevant, 'simple' or something else?

Also where this variable is defined in the code, put in a comment what the differences are in the bindings and use of the metatable.

The Travis build failed... did you get an email about that? Travis tests need to pass before a merge can be contemplated. The failures look like the seg faults I mentioned on 1 Feb. Can you also address all the comments from my 1 Feb posting please?

Contributor

v-for-vandal commented Feb 19, 2014

Of course, I understand that Travis should pass before merge can occur :) I didn't neglect those segfaults, (btw, many thanks for backtrace. I wasn't able to reproduce this problem and without your backtrace it would have been unsolvable.), I had just started from another issue.
Could you wait til the next week with the merge ? The nearest weekends is the closet possible time slot when I will be able to fully concentrate on fixing all remaining issues.

Contributor

v-for-vandal commented Feb 22, 2014

Hi @wsfulton .

  1. Thanks to your backtrace, I was able to fix this annoying segfault issue :)
  2. Renamed cmd options and requested variables
  3. I still stand by my decision to avoid repeated usage of Swig_name_* and remember the generated wrapper name as properties of the node instead.
  4. Regarding mangled_fq_class_symname: it's primary use is in naming swig_lua_class structure that holds all information about class as members. It's naming scheme is "_wrap_class" + $(fully qualified class name), and mangling is necessary to form a valid C identifier.
    1. If you are talking about lua.cxx:1301 then it's usage in "destructor_name" creation is of secondary importance (although with same motivation - generate unique identifier). It is not my code and I left it be. The generated function "swig_delete_" + $mangled_fq_class_symname is not - as far as I can see - a wrapper around destructor, but a wrapper around a Swig_name_destroy-generated wrapper around destructor. The reason for such decision is unknown to me.
  5. Part 2 was going to be a director support. But director support requires moving to the standard typemaps library and it had proven to be more time consuming than I thought. I am currently half done with typemaps, but no way I can finish before SWIG 3.0. I don't think that it will be backward-incompatible change any way. So no second part for now, sorry (
  6. Did I miss any of your questions / issues ?
  7. As usual, I am ready to provide more detailed explanations and fix the required issues.
Owner

wsfulton commented Feb 22, 2014

Looks better after a cursory look. I'll add comments one by one as I go through this in more detail.

First can you add the documentation for -no-old-metatable-bindings. Preferably with some examples to explain.

Contributor

v-for-vandal commented Feb 22, 2014

I've update documentaion (Lua.html) and renamed -swig3 to it's new name. Is this what you meant, or you would like me to put more info into help message ?
May be it would be appropriate to add something like "For exact information about affected structures and other changes see documentation" instead of duplicating all those changes in the help message ?

Owner

wsfulton commented Feb 22, 2014

Yes, the new .html doc update suffices. No need to change the -help to refer to the docs.

Does this patch completely supercede #62?

I agree with your comments about directors.

Contributor

v-for-vandal commented Feb 22, 2014

Yes, it does.

Owner

wsfulton commented Feb 22, 2014

Havn't you lost the ability for users to override MIN_OPT_LEVEL or is there no need to do that now? I thought you needed this:

#ifndef MIN_OPT_LEVEL
#define MIN_OPT_LEVEL 2
#endif

wsfulton referenced this pull request Feb 22, 2014

Closed

Classobj r2 #62

Contributor

v-for-vandal commented Feb 22, 2014

Hm. I'll see to it tomorrow morning (it is 3:30am here), run eLua tests locally and commit fixes if necessary, ok?

Owner

wsfulton commented Feb 22, 2014

Could you please convert a runtime test of nspace_extend from one of the other languages... just to add extra verification that %nspace is good, as the testing is just the one testcase atm?

Owner

wsfulton commented Feb 22, 2014

Sure no need to stay up that late!

Owner

wsfulton commented Feb 23, 2014

The only real problem left now is lua:name. Probably the original implementation of the Lua module is at fault, but I think that the setting of lua:name on the nodes is not right. The Swig_name_* functions are not being used in the Lua module as they should be and the original mistake of not using them has resulted in you having to make the situation worse. I'm afraid I'd like this fixed to work like the other modules as it is likely to break various subtle features and is not currently maintainable by a regular SWIG developer. I appreciate this might take a bit of effort and I'm willing to wait and even delay 3.0.0 for another week or whatever it takes. You'll probably find that directors will be easier to support if you are following the norm as you can utilise the common code / copy&paste from other language modules.

You mentioned that Language::staticmemberfunctionHandler doesn't use NSpace. It does:

  mrename = Swig_name_member(NSpace, ClassPrefix, symname);

and it has been like that since 2010, so I don't understand what version of code you are looking at.

If NSpace is not working in Language in any way, of course we should fix it.

Contributor

v-for-vandal commented Feb 23, 2014

  1. Added nspace_extend test case
  2. MIN_OPT_LEVEL Yes, you are most likely right. I added it to the luarun.swg. Anyway eLua support is murky, there is no way to run tests etc. I will try to tackle it later, after merge, may be I'wll try to contact Ramon Gopalan again.
  3. lang.cxx:1573. Sorry, it was Language::staticmembervariableHandler. I always mix those two.
  4. Ok, I wil switch to Swig_name_* methods. Shouldn't take long.
Owner

wsfulton commented Feb 23, 2014

Okay, great, thanks. eLua support is lowest priority IMHO.

I hope the Swig_name_* support isn't too tricky. In case you don't know, a tip for testing refactored code is to use 'make partial-check-lua-test-suite'. This just invokes SWIG and so is much faster. You can then use a recursive diff or gui diff program (eg meld) to compare a snapshot of the Examples/test-suite/lua directory with the output of running the partialcheck. Then the diffs show where you have perhaps made mistakes in the refactoring.

Contributor

v-for-vandal commented Feb 25, 2014

Hi @wsfulton

I switched module to Swig_name_. Wasn't nearly as hard as my first attempt to do this. Any comments to suggested solution ?
I moved all code to registerVariable/registerMethod functions. They look whether variable(function) is static, global or member one and derive correct wrapper name based on this information (using Swig_name_
methods).

And what is the problem with lua:name ? It is just a name that function/variable should have in Lua. It has nothing to do with C++ code generation. It has multiple purposes:

  1. It is immune to all that mess transformations that Language functions do with sym:name. Usefull for debugging and when you need to get correct name while sym:name is transformed.
  2. Sometimes I need to register one function under more than one name (backward compatibility). Changing sym:name won't work because Swig_name_* will generate wrong setter/getter/wrapper names.

Any more things I should fix ? (beside removing all TODO:REMOVE)

Owner

wsfulton commented Feb 26, 2014

I'll try look at this tomorrow. I still don't understand why symname doesn't work for you. You shouldn't need to override cDeclaration and lua:name functionality should be obtained from symname and the getClassName(), getClassPrefix(), etc in Language as well as sym:nspace. You should have something like JAVA::getProxyName().

Contributor

v-for-vandal commented Feb 26, 2014

Consider the following example:
I have class C1 variable with sym:name "instanceVariable". It's wrappers - generated with Swig_name_* are
_wrap_C1_instanceVariable_get and _wrap_C1_instanceVariable_set.
Variable registration - an array entry - looks like this:
{ "lua name", getter, setter }
I need to add those to statements to generated C++ file:
main variable record with name "instanceVariable":

{ "instanceVariable", $Swig_name_get(..."instanceVariable"..), $Swig_name_set(..."instanceVariable"...) }

and alternative variable record with backward compatible name, say, "backwardCompatVarName":

{ "backwardCompatVarName", ${Swig_name_get(..."instanceVariable"..), Swig_name_set(..."instalceVariable"...) }.

If I use sym:name instead of lua:name, then when I change sym:name to "backwardCompatVarName", then result of Swig_name_(${sym:name}) will change as well. And that is incorrect.
If I manipulate with lua:name instead then Swig_name_
always return correct function names and the only thing that changes is under what name this variable is represented in Lua.

Owner

wsfulton commented Feb 28, 2014

@v-for-vandal I don't see any more fundamental problems, so will merge this to master. There are a few cosmetics which I will do. There are also a couple more minor things I'd like you to sort out as described below. Will you have time to you work on them this weekend? If so, I'll merge this to master which will be easier for us to both work on the code.

The Swig_name_* functionality looks good. Your usage of lua:name to work around sym;name changes in Language is much more widely spread than in other languages. It needs a better solution in Language, so until then, it can stay as is.

Contributor

v-for-vandal commented Feb 28, 2014

Yes, unless some emergency transpired, I will be able to work on any desired changes this weekend.

wsfulton merged commit c6dd6b0 into swig:master Feb 28, 2014

1 check passed

default The Travis CI build passed
Details
Owner

wsfulton commented Feb 28, 2014

This is in master now. Can you open a new pull request addressing the following issues please:

Something is wrong in the documentation or the implementation is faulty regarding the Lua Namespaces section. I get nil for the following:
print(example.MyWorld.Nested.Dweller.MALE)
print(example.MyWorld.Nested.Dweller.food_count)
Can you check this section please. There were lots of typos, it is best if you copy code that actually runs.

Regarding the "Constants/enums and classes/structures" section and C code in the docs:

static const int ICONST = 12;

within a struct is not valid C code, so example.ICONST will never work.

struct Test {
    enum { TEST1 = 10, TEST2 = 20 };
    static const int ICONST = 12;
};

Test.TEST1 should not work when wrapping C code either. This is wrong as TEST1 is in the global namespace NOT within Test. We discussed a similar problem for nested C structs and you need to change enums declared within a struct to be in the global Lua namespace. This will also mean not making an unnecessary change for C code prior to 3.0.0.

Can you also change your H4 "Backward compatibility" sections into "Compatibility Note" paragraphs like in the rest of the SWIG docs. I started doing this for you with what I committed.

Please tidy up, all the "TODO : REMOVE" in luarun.swg and lua.cxx

__Static ... change to SwigStatic please... __Static is illegal in C++ and fix the comments in lua.cxx about this.
Similarly for __Module - change to SwigModule
Can you please use SetFlag instead of Setattr for the new flags you've added.

wsfulton referenced this pull request Mar 4, 2014

Merged

Class p1 fixes #144

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment