Fix python global object constants

[ptomulik]

With current swig the following simple test case

%module object_constants

struct Type1 { };
%constant Type1 t1;

%{
  struct Type1 {};
  Type1 t1;
%}

does not compile, when generated with -builtin option. The compilation error is:

swig -python -c++ -builtin -o object_constants_wrap.cxx object_constants.i
g++ -o object_constants_wrap.o -c -fpic -I.  -I/usr/include/python2.7 object_constants_wrap.cxx 
object_constants_wrap.cxx: In function ‘void init_object_constants()’:
object_constants_wrap.cxx:1170:83: error: ‘self’ was not declared in this scope
 #define SWIG_NewPointerObj(ptr, type, flags)            SWIG_Python_NewPointerObj(self, ptr, type, flags)
                                                                                   ^
object_constants_wrap.cxx:4530:70: note: in expansion of macro ‘SWIG_NewPointerObj’
   SWIG_Python_SetConstant(d, d == md ? public_interface : NULL, "t1",SWIG_NewPointerObj(SWIG_as_voidptr(&t1),SWIGTYPE_p_Type1, 0 ));

This patch fixes the problem.

Advice needed: I've set self to m (module) but maybe it should be NULL? Please advise.

[wsfulton]

%constant and structs isn't really supported from what I can see. I'm not convinced there is a great need for them either. Nevertheless the 3 commits mentioned above improve matters and should get them working for the vast majority of languages.

Although your patch fixes the problem for python -builtin, I added in a test case in 70a04c9. The corresponding runtime testcase should be in constant_directive_runme.py as follows:

import constant_directive

print("TYPE1_CONSTANT1    type: {}".format(type(constant_directive.TYPE1_CONSTANT1)))
print("getType1Instance() type: {}".format(type(constant_directive.getType1Instance())))

if constant_directive.TYPE1_CONSTANT1.val != 1:
  raise RuntimeError("fail")
if constant_directive.TYPE1_CONSTANT2.val != 2:
  raise RuntimeError("fail")
if constant_directive.TYPE1_CONSTANT3.val != 3:
  raise RuntimeError("fail")

which does work for -builtin, but fails using default options. The output is

checking python testcase constant_directive (with run test)
TYPE1_CONSTANT1    type: <type 'SwigPyObject'>
getType1Instance() type: <class 'constant_directive.Type1'>
Traceback (most recent call last):
  File "constant_directive_runme.py", line 6, in <module>
    if constant_directive.TYPE1_CONSTANT1.val != 1:
AttributeError: 'SwigPyObject' object has no attribute 'val'

I think there is some sort of initialisation ordering problem registering the types, but I havn't looked further and don't have the inclination to do so atm.

[ptomulik]

@wsfulton, thanks for looking at the PR. From what I remember, I needed these constant structs for enum classes/strong enums implemented in #251. I'll adjust this PR according to your suggestions.

[ptomulik]

@wsfulton, I think I found the cause for struct constant having no user-defined attributes in python. This is about how constants are spawned. This is what I see for the struct-constants in constant_directive.py generated by swig:

TYPE1_CONSTANT1 = _constant_directive.TYPE1_CONSTANT1
# ...

The _constant_directive.TYPE1_CONSTANT1 however has no attributes defined in constant_directive.i. The attributes appear only for variables created in python by spawning new instances of class Type1 (shadow class). In the generated shadow file constant_directive.py we have:

class Type1(_object):                                                           
    __swig_setmethods__ = {}                                                    
    __setattr__ = lambda self, name, value: _swig_setattr(self, Type1, name, value)
    __swig_getmethods__ = {}                                                    
    __getattr__ = lambda self, name: _swig_getattr(self, Type1, name)           
    __repr__ = _swig_repr                                                       

    def __init__(self, val=0):                                                  
        this = _constant_directive.new_Type1(val)                               
        try:                                                                    
            self.this.append(this)                                              
        except:                                                                 
            self.this = this                                                    
    __swig_setmethods__["val"] = _constant_directive.Type1_val_set              
    __swig_getmethods__["val"] = _constant_directive.Type1_val_get              
    if _newclass:                                                               
        val = _swig_property(_constant_directive.Type1_val_get, _constant_directive.Type1_val_set)
    __swig_destroy__ = _constant_directive.delete_Type1                         
    __del__ = lambda self: None                                                 
Type1_swigregister = _constant_directive.Type1_swigregister                     
Type1_swigregister(Type1)                                                       

An example python variable created by:

t = Type1();

will have all the attributes exposed to python, but

_constant_directive.TYPE1_CONSTANT1

lacks the __swig_getmethods__, __swig_setmethods__, __getattr__, __setattr__, etc.

Shouldn't swig create struct contants by creating "fully-fledged" python variables instead of references to some internal stuff? What I try to say is to do this:

TYPE1_CONSTANT1 = Type1(1)
# ...

in the shadow file.

[ptomulik]

Ok, I have to correct myself. After digging a little I came to conclusion, that without -builtin the constants should be set after the

Type1_swigregister(Type1)

so the current approach of setting the constants inside of SWIG_init has to be changed.

[wsfulton]

Yes, that is what I suspected, the constants should be created after the initialisation/registration of the types.

[ptomulik]

Got it working (with, and without -builtin). With no -builtin it writes <CONSTANT_NAME>_swigregister() functions to *_wrap.cxx and calls them from the python shadow file (this happens to be done after <CONSTANT_TYPE>_swigregister() is called, so we have the required types complete). There are some exceptions, when it's not done this way, and then the constant creation goes normally to the SWIG_init function within the generated *_wrap.cxx file. The exceptions include all the cases, where it's not possible or suitable to write to the shadow file (based on how it was implemented previously).

[ptomulik]

I think it may be reviewed and eventually merged. For some reason, the octave build failed on travis-ci (gcc internal compiler error), but I think it's not related to this PR.

[wsfulton]

The Octave build sometimes fails and is irrelevant to this.

Without -builtin, the following in the constant_directive.i testcase:

%constant Type1 TYPE1_CONSTANT1;

generates:

_constant_directive.TYPE1_CONSTANT1_swigregister(_constant_directive)
TYPE1_CONSTANT1_swigregister = _constant_directive.TYPE1_CONSTANT1_swigregister
TYPE1_CONSTANT1 = _constant_directive.TYPE1_CONSTANT1

I can't see the point of the 2nd line. Have I missed something?

The TYPE1_CONSTANT1_swigregister function isn't registering types like Type1_swigregister is. I suggest it uses a different suffix, eg _swigconstant.

In the generated code, can you replace 'm' with 'module' to be clearer what it is. Can you adapt it to use the standard code formatting spacing , eg PyObject * instead of PyObject* and 'if (' instead of 'if('.

Can you also ensure it works when using the various python optimisation flags (-O -modernargs -classic etc).

Can you also modify constant_directive_runme.py and move the info in the print statements into the RuntimeError.

[ptomulik]

I've seen similar line to the second in the *_swigregister calls for types. I thought there is some convention to "import" names of *_swigregister (and such) functions to shadow files, so just followed this. I agree it would be better to remove this line, so I'm going to do this quickly. I'll also change swigregister to swigconstant and follow other adwices too.

[ptomulik]

I have updated this PR following all your suggestions. I've also ran tests for some arbitrarily choosen combinations of swig flags. My test run script and the results may be found here. There is some problem with -classic, probably the constant_directive_runme.py script has to be adjusted (I believe, the instances of classic clases are seen quite differently in python type system than instances of modern classes). I've also seen, that all the tests having runtime scripts (*_runme.py) segfault when -modernargs flag is used with -builtin.

[ptomulik]

Ok, I have fixed the issue with -classic. The -modernargs + -builtin should be addressed in separate PR as it's not related to this particular PR. I think, this PR is ready to be merged.

[wsfulton]

-builtin should not be used with many of the other flags like -modern, but then again, SWIG shouldn't seg fault if this is attempted! Thanks, for the update, I'm about to merge them in.

[ptomulik]

Thank you for merging. I just want to make a remark about segfault. It happens when we use -builtin -modernargs. However, if we add to this duet the -fastunpack, for example, it passes tests again.