[OPW] Generated swapping and size-checking code integration

[chris at demorecorder.com]

Hi Asalle and Peter,

I add some comments to your (Peter's) comments about the Python code.

Peter Hutterer wrote on 17.02.2015 08:36:
>> --- /dev/null
>> +++ b/proto/gen_swap_check.py
>> @@ -0,0 +1,753 @@
[...]
>> +
>> +# Some hacks to make the API more readable and to keep backwards compability
>> +_cname_special_cases = {'DECnet':'decnet'}
>> +_extension_special_cases = ['XPrint', 'XCMisc', 'BigRequests']
>> +_cname_re = re.compile('([A-Z0-9][a-z]+|[A-Z0-9]+(?![a-z])|[a-z]+)')
> 
> please add a comment to describe what the regex should do (roughly anyway).
> I can probably figure this out with a bit of time, but then I likely still
> won't know whether it's what you intended to do :)
That's copied from xcb/libxcb/src/c_client.py, so any comment should also go
there.
( or we put the common code in an extra Python module to avoid
the duplicates. This however will require lots of changes because the
common code is all private functions starting with "_", so they all need to be
renamed.)

>> +
>> +def _increase_indent():
>> +	global _indent
>> +	_indent.append('    ')
>> +	
>> +def _decrease_indent():
>> +	global _indent
>> +	if _indent: # if not empty
>> +		_indent.pop()
>> +
>> +# XXX See if this level thing is really necessary.
>> +def _h_setlevel(idx):
>> +    '''
>> +    Changes the array that header lines are written to.
>> +    Supports writing different sections of the header file.
>> +    '''
>> +    global _hlevel
>> +    while len(_hlines) <= idx:
>> +        _hlines.append([])
>> +    _hlevel = idx
>> +
>> +def _c_setlevel(idx):
>> +    '''
>> +    Changes the array that source lines are written to.
>> +    Supports writing to different sections of the source file.
>> +    '''
>> +    global _clevel
>> +    while len(_clines) <= idx:
>> +        _clines.append([])
>> +    _clevel = idx
>> +    
>> +def _n_item(str):
> 
> "n_item" usually refers to "number of items". can we rename this into
> something more expressive?
> same goes for _ext, _n and _t, no idea what they do based on the function
> name alone.

Also copied from libxcb/src/c_client.py
These functions probably have such short names because they are used at many
places to generate code.
But I still think that longer function-names which describe the semantics would
still be better. 
( except for _c and _h, which are short and clear in what they do: output to *.c
and *.h files )

It is consensus on the xcb-list that the code-quality of c_client.py has a lot of
room for improvement.
The naming of these functions is one of the low prio issues compare to the other
stuff... :-)


> 
>> +    '''
>> +    Does C-name conversion on a single string fragment.
> 
> an example of the conversion would be nice here (same below)
Still copied code.

[...]
>> +def c_simple(self, name):
>> +	'''
>> +	Exported function that handles cardinal type declarations.
>> +    These are types which are typedef'd to one of the CARDx's, char, float,
>> etc.
>> +	'''
>> +	#print(name)
>> +	#print(self)
> 
> can we drop the functions that don't do anything? Or at least comment why
> they don't do anything?
It needs to be commented.
These functions cannot be dropped because they are used as function references
in a hash further below. 
These are handler functions called by the parser.
The parser is in xcb/proto/xcbgen. 

[...]
>> +def c_request(self, name):
>> +	'''
>> +	Exported function that handles request declarations.
>> +	''' 
>> +	_h_setlevel(1)
>> +	_c_setlevel(1)
>> +	global requests
>> +	requests[self.opcode] = name
>> +	for kind in swapCheckDescendants:
>> +		swapcheck = kind(RequestHandler())
>> +		swapcheck.generate(self, name)
>> +		
>> +swapCheckDescendants = {SwapFromClient, SwapToClient, Check}
> 
> move this to the top of the file somewhere pls

This cannot be moved to the top of the file because
the members of this list are classes.
(classes are first-class objects in Python, so they
can be used in lists etc).

The classes need to be defined before they can be used in this list.
So the list has be defined rather near the end of the file.

> 
>> +	
>> +def c_event(self, name):
>> +	'''
>> +	Exported function that handles event declarations.
>> +	'''
>> +	#print(name)
>> +	
>> +def c_error(self, name):
>> +	'''
>> +	Exported function that handles error declarations.
>> +	'''
>> +	#print(self)
>> +	#print(name)
> 
> hmm, so gut feeling tells me that you probably want some error message here
> so you don't accidentally create swapping code that skips events or errors.
These are handlers that shall generate code for X11-events and X11-errors.
The generator is a set of handlers that are called by the parser
(xcb/proto/xcbgen).

X11-Events and X11-errors and not yet generated by this generator.


> 
>> +	
>> +# Must create an "output" dictionary before any xcbgen imports.
>> +output = {'open'    : c_open,
>> +          'close'   : c_close,
>> +          'simple'  : c_simple,
>> +          'enum'    : c_enum,
>> +          'struct'  : c_struct,
>> +          'union'   : c_union,
>> +          'request' : c_request,
>> +          'event'   : c_event,
>> +          'error'   : c_error,
>> +          }
>> +
>> +# Check for the argument that specifies path to the xcbgen python package.
>> +try:
>> +    opts, args = getopt.getopt(sys.argv[1:], 'p:')
>> +except getopt.GetoptError as err:
>> +    print(err)
>> +    print('Usage: gen_swap_check.py [-p path] file.xml')
>> +    sys.exit(1)
>> +
>> +for (opt, arg) in opts:
>> +    if opt == '-p':
>> +        sys.path.insert(1, arg)
>> +
>> +# Import the module class
>> +try:
>> +    from xcbgen.state import Module
>> +    from xcbgen.xtypes import *
>> +except ImportError:
>> +    print('''
>> +Failed to load the xcbgen Python package!
>> +Make sure that xcb/proto installed it on your Python path.
>> +If not, you will need to create a .pth file or define $PYTHONPATH
>> +to extend the path.
>> +Refer to the README file in xcb/proto for more info.
>> +''')
>> +    raise
>> +
>> +module = Module(args[0], output)
>> +module.register()
>> +module.resolve()
>> +module.generate()
> 
> this is the C programmer in me, but I've really started liking the python
> approach of:
> 
> def main(argv):
>     # do stuff
> 
> if __name__ == "__main__":
>        main(sys.argv)
> 
> IMO it nicely groups the code to run when the file is executed.

Interesting method. Looks attractive for the C/C++ programmer in me, too. ;-)

That code is also copied from c_client.py.

All in all, most of your comments are about code which was copied
from xcb/libxcb/src/c_client.py. 
So, your comments apply there as well.

I think we first need to decide whether we keep the code-duplication
or whether we create a python module with the common code.
There are advantages for each approach.

It's basically 
  "reduce code-duplication"
vs
  "avoid API-compatibility issues of public APIs" ( we'd need to introduce a
  public api of that common code. )
We cannot have both.

Then we can fix these issues.

Cheers,

Chris