[PATCH] Byte-swapping and size-checking generator all-in-one
Peter Hutterer
peter.hutterer at who-t.net
Tue Mar 24 00:13:47 PDT 2015
Hi Asal,
On Tue, Mar 24, 2015 at 08:16:44AM +0200, Asal Mirzaieva wrote:
> Good morning all and Peter,
>
> I replied to your comments below.
>
> On 17.03.15 09:25, Peter Hutterer wrote:
[...]
> >>diff --git a/configure.ac b/configure.ac
> >>index 96524c5..5b89705 100644
> >>--- a/configure.ac
> >>+++ b/configure.ac
> >>@@ -33,6 +33,7 @@ AC_CONFIG_SRCDIR([Makefile.am])
> >> AC_CONFIG_MACRO_DIR([m4])
> >> AM_INIT_AUTOMAKE([foreign dist-bzip2])
> >> AC_USE_SYSTEM_EXTENSIONS
> >>+AM_PATH_PYTHON([2.7])
> >> # Require xorg-macros minimum of 1.14 for XORG_COMPILER_BRAND in XORG_DEFAULT_OPTIONS
> >> m4_ifndef([XORG_MACROS_VERSION],
> >>@@ -576,6 +577,7 @@ AC_ARG_WITH(khronos-spec-dir, AS_HELP_STRING([--with-khronos-spec-dir=PATH], [Pa
> >> [KHRONOS_SPEC_DIR=auto])
> >> dnl Extensions.
> >>+AC_ARG_ENABLE(proto, AS_HELP_STRING([--disable-composite], [Build Proto (default: enabled)]), [PROTO=$enableval], [PROTO=yes])
> >I'm gonna go out on a limb here and say that the help string isn't quite
> >what you were looking for :)
> >fwiw, "proto" is a rather generic term, I'd use xcb-proto-parsing or
> >something. But really, I think this should be unconditional anyway
> >so you don't need this flag at all.
> You mean, I don't need AC_ARG_ENABLE? I just don't get how to intergrate the
> newly written code to already existing one another way.
assume that PROTO=yes, always. and then write the configure.ac changes that
way...
> >
> >> AC_ARG_ENABLE(composite, AS_HELP_STRING([--disable-composite], [Build Composite extension (default: enabled)]), [COMPOSITE=$enableval], [COMPOSITE=yes])
> >> AC_ARG_ENABLE(mitshm, AS_HELP_STRING([--disable-mitshm], [Build SHM extension (default: auto)]), [MITSHM=$enableval], [MITSHM=auto])
> >> AC_ARG_ENABLE(xres, AS_HELP_STRING([--disable-xres], [Build XRes extension (default: enabled)]), [RES=$enableval], [RES=yes])
> >>@@ -1036,6 +1038,13 @@ if test "x$COMPOSITE" = xyes; then
> >> COMPOSITE_INC='-I$(top_srcdir)/composite'
> >> fi
> >>+AM_CONDITIONAL(PROTO, [test "x$PROTO" = xyes])
> >>+if test "x$PROTO" = xyes; then
> >>+ AC_DEFINE(PROTO, 1, [Support Proto])
> >>+ PROTO_LIB='$(top_builddir)/proto/libproto.la'
> >>+ PROTO_INC='-I$(top_srcdir)/proto/generated'
> >>+fi
so you skip the AM_CONDITIONAL, if test, etc. and just have the three line
from the condition itself.
> >>+
> >> if test "x$MITSHM" = xauto; then
> >> MITSHM="$ac_cv_sysv_ipc"
> >> fi
> >>@@ -1776,7 +1785,26 @@ AC_EGREP_CPP([I_AM_SVR4],[
> >> AC_DEFINE([SVR4],1,[Define to 1 on systems derived from System V Release 4])
> >> AC_MSG_RESULT([yes])], AC_MSG_RESULT([no]))
> >>-XSERVER_CFLAGS="$XSERVER_CFLAGS $CORE_INCS $XEXT_INC $COMPOSITE_INC $DAMAGE_INC $FIXES_INC $XI_INC $MI_INC $MIEXT_SYNC_INC $MIEXT_SHADOW_INC $MIEXT_LAYER_INC $MIEXT_DAMAGE_INC $RENDER_INC $RANDR_INC $FB_INC $DBE_INC $PRESENT_INC"
> >>+XSERVER_CFLAGS="$XSERVER_CFLAGS $CORE_INCS $XEXT_INC $COMPOSITE_INC $DAMAGE_INC $FIXES_INC $XI_INC $MI_INC $MIEXT_SYNC_INC $MIEXT_SHADOW_INC $MIEXT_LAYER_INC $MIEXT_DAMAGE_INC $RENDER_INC $RANDR_INC $FB_INC $DBE_INC $PRESENT_INC $PROTO_INC"
> >>+
> >>+dnl ---------------------------------------------------------------------------
> >>+dnl proto section.
> >>+dnl ---------------------------------------------------------------------------
> >>+
> >>+# Find the xcb-proto protocol descriptions
> >>+PKG_CHECK_MODULES([XCBPROTO], [xcb-proto])
> >>+XCBPROTO_XCBINCLUDEDIR=`$PKG_CONFIG --variable=xcbincludedir xcb-proto`
> >>+AC_SUBST(XCBPROTO_XCBINCLUDEDIR)
> >>+
> >>+# Find the xcbgen Python package
> >>+AC_MSG_CHECKING(XCBPROTO_XCBPYTHONDIR)
> >>+XCBPROTO_XCBPYTHONDIR=`$PKG_CONFIG --variable=pythondir xcb-proto`
> >>+AC_MSG_RESULT($XCBPROTO_XCBPYTHONDIR)
> >>+AC_SUBST(XCBPROTO_XCBPYTHONDIR)
> >>+
> >>+# CFLAGS
> >>+PROTO_CFLAGS="$XSERVER_CFLAGS $XCBPROTO_CFLAGS"
> >>+AC_SUBST([PROTO_CFLAGS])
> >> dnl ---------------------------------------------------------------------------
> >> dnl DDX section.
> >>@@ -1789,7 +1817,7 @@ AC_MSG_RESULT([$XVFB])
> >> AM_CONDITIONAL(XVFB, [test "x$XVFB" = xyes])
> >> if test "x$XVFB" = xyes; then
> >>- XVFB_LIBS="$FB_LIB $FIXES_LIB $XEXT_LIB $DBE_LIB $RECORD_LIB $GLX_LIBS $RANDR_LIB $RENDER_LIB $DAMAGE_LIB $DRI3_LIB $PRESENT_LIB $MIEXT_SYNC_LIB $MIEXT_DAMAGE_LIB $MIEXT_SHADOW_LIB $XI_LIB $XKB_LIB $XKB_STUB_LIB $COMPOSITE_LIB"
> >>+ XVFB_LIBS="$FB_LIB $FIXES_LIB $XEXT_LIB $DBE_LIB $RECORD_LIB $GLX_LIBS $RANDR_LIB $RENDER_LIB $DAMAGE_LIB $DRI3_LIB $PRESENT_LIB $MIEXT_SYNC_LIB $MIEXT_DAMAGE_LIB $MIEXT_SHADOW_LIB $XI_LIB $XKB_LIB $XKB_STUB_LIB $COMPOSITE_LIB $PROTO_LIB"
> >> XVFB_SYS_LIBS="$XVFBMODULES_LIBS $GLX_SYS_LIBS"
> >> AC_SUBST([XVFB_LIBS])
> >> AC_SUBST([XVFB_SYS_LIBS])
> >>@@ -1810,7 +1838,7 @@ if test "x$XNEST" = xyes; then
> >> if test "x$have_xnest" = xno; then
> >> AC_MSG_ERROR([Xnest build explicitly requested, but required modules not found.])
> >> fi
> >>- XNEST_LIBS="$FB_LIB $FIXES_LIB $MI_LIB $XEXT_LIB $DBE_LIB $RECORD_LIB $GLX_LIBS $RANDR_LIB $RENDER_LIB $DAMAGE_LIB $DRI3_LIB $PRESENT_LIB $MIEXT_SYNC_LIB $MIEXT_DAMAGE_LIB $MIEXT_SHADOW_LIB $XI_LIB $XKB_LIB $XKB_STUB_LIB $COMPOSITE_LIB $MAIN_LIB $DIX_LIB $OS_LIB"
> >>+ XNEST_LIBS="$FB_LIB $FIXES_LIB $MI_LIB $XEXT_LIB $DBE_LIB $RECORD_LIB $GLX_LIBS $RANDR_LIB $RENDER_LIB $DAMAGE_LIB $DRI3_LIB $PRESENT_LIB $MIEXT_SYNC_LIB $MIEXT_DAMAGE_LIB $MIEXT_SHADOW_LIB $XI_LIB $XKB_LIB $XKB_STUB_LIB $COMPOSITE_LIB $MAIN_LIB $DIX_LIB $OS_LIB $PROTO_LIB"
> >> XNEST_SYS_LIBS="$XNESTMODULES_LIBS $GLX_SYS_LIBS"
> >> AC_SUBST([XNEST_LIBS])
> >> AC_SUBST([XNEST_SYS_LIBS])
> >>@@ -1835,7 +1863,7 @@ if test "x$XORG" = xyes; then
> >> XORG_OSINCS='-I$(top_srcdir)/hw/xfree86/os-support -I$(top_srcdir)/hw/xfree86/os-support/bus -I$(top_srcdir)/os'
> >> XORG_INCS="$XORG_DDXINCS $XORG_OSINCS"
> >> XORG_CFLAGS="$XORGSERVER_CFLAGS -DHAVE_XORG_CONFIG_H"
> >>- XORG_LIBS="$COMPOSITE_LIB $FIXES_LIB $XEXT_LIB $DBE_LIB $RECORD_LIB $RANDR_LIB $RENDER_LIB $DAMAGE_LIB $DRI3_LIB $PRESENT_LIB $MIEXT_SYNC_LIB $MIEXT_DAMAGE_LIB $XI_LIB $XKB_LIB"
> >>+ XORG_LIBS="$COMPOSITE_LIB $FIXES_LIB $XEXT_LIB $DBE_LIB $RECORD_LIB $RANDR_LIB $RENDER_LIB $DAMAGE_LIB $DRI3_LIB $PRESENT_LIB $MIEXT_SYNC_LIB $MIEXT_DAMAGE_LIB $XI_LIB $XKB_LIB $PROTO_LIB"
> >> dnl ==================================================================
> >> dnl symbol visibility
> >>@@ -2391,11 +2419,11 @@ if test "$KDRIVE" = yes; then
> >> KDRIVE_INC='-I$(top_srcdir)/hw/kdrive/src'
> >> KDRIVE_PURE_INCS="$KDRIVE_INC $MIEXT_SYNC_INC $MIEXT_DAMAGE_INC $MIEXT_SHADOW_INC $XEXT_INC $FB_INC $MI_INC"
> >> KDRIVE_OS_INC='-I$(top_srcdir)/hw/kdrive/linux'
> >>- KDRIVE_INCS="$KDRIVE_PURE_INCS $KDRIVE_OS_INC"
> >>+ KDRIVE_INCS="$KDRIVE_PURE_INCS $KDRIVE_OS_INC $PROTO_INC"
> >> KDRIVE_CFLAGS="$XSERVER_CFLAGS -DHAVE_KDRIVE_CONFIG_H $TSLIB_CFLAGS"
> >>- KDRIVE_PURE_LIBS="$FB_LIB $MI_LIB $FIXES_LIB $XEXT_LIB $DBE_LIB $RECORD_LIB $GLX_LIBS $RANDR_LIB $RENDER_LIB $DAMAGE_LIB $DRI3_LIB $PRESENT_LIB $MIEXT_SYNC_LIB $MIEXT_DAMAGE_LIB $MIEXT_SHADOW_LIB $XI_LIB $XKB_LIB $XKB_STUB_LIB $COMPOSITE_LIB $OS_LIB"
> >>+ KDRIVE_PURE_LIBS="$FB_LIB $MI_LIB $FIXES_LIB $XEXT_LIB $DBE_LIB $RECORD_LIB $GLX_LIBS $RANDR_LIB $RENDER_LIB $DAMAGE_LIB $DRI3_LIB $PRESENT_LIB $MIEXT_SYNC_LIB $MIEXT_DAMAGE_LIB $MIEXT_SHADOW_LIB $XI_LIB $XKB_LIB $XKB_STUB_LIB $COMPOSITE_LIB $OS_LIB $PROTO_LIB"
> >> KDRIVE_LIB='$(top_builddir)/hw/kdrive/src/libkdrive.la'
> >> case $host_os in
> >> *linux*)
> >>@@ -2546,6 +2574,7 @@ xfixes/Makefile
> >> exa/Makefile
> >> dri3/Makefile
> >> present/Makefile
> >>+proto/Makefile
just spotted this one too, this seems like a whitespace issue too
> >> hw/Makefile
> >> hw/xfree86/Makefile
> >> hw/xfree86/Xorg.sh
> >>diff --git a/proto/.gitignore b/proto/.gitignore
> >>new file mode 100644
> >>index 0000000..cc4611b
> >>--- /dev/null
> >>+++ b/proto/.gitignore
> >>@@ -0,0 +1,5 @@
> >>+# Python "compiled" files
> >>+*.pyc
> >>+
> >>+# Autogenerated by gen_swap_check.py in proto files
> >>+gen/*
> >>diff --git a/proto/Makefile.am b/proto/Makefile.am
> >>new file mode 100644
> >>index 0000000..34af921
> >>--- /dev/null
> >>+++ b/proto/Makefile.am
> >>@@ -0,0 +1,21 @@
> >>+noinst_LTLIBRARIES = libproto.la
> >>+
> >>+AM_CFLAGS = $(PROTO_CFLAGS)
> >>+
> >>+prefx=swapcheck_
> >is prefix a reserved word? or do we just disagree about the spelling? :)
> I thought, that it can be a reserved word or a variable somewhere else, so I
> didn't want to risk. I guess, I should have written something like
> xcb_proto_parsing_prefix = swapcheck_
yep, sounds good. in this day and age we can afford long variable names,
storage costs are down this week ;)
> >>+PROTO_GENERATEDDIR=generated
> >>+
> >>+extension_sources = \
> >>+ ${PROTO_GENERATEDDIR}/${prefx}xproto.c \
> >>+ ${PROTO_GENERATEDDIR}/${prefx}shape.c
> >>+
> >>+libproto_la_SOURCES = $(extension_sources)
> >>+
> >>+sources = $(subst ${PROTO_GENERATEDDIR}/${prefx}, ,$(@))
> >>+
> >>+$(extension_sources): $(XCBPROTO_XCBINCLUDEDIR)/$(sources:.c=.xml) $(srcdir)/gen_swap_check.py
> >>+
> >you can drop this empty line here
> >
> >>+ $(AM_V_GEN)$(PYTHON) $(srcdir)/gen_swap_check.py \
> >>+ -p $(XCBPROTO_XCBPYTHONDIR) \
> >>+ -d ${PROTO_GENERATEDDIR} \
> >>+ $(XCBPROTO_XCBINCLUDEDIR)/$(sources:.c=.xml)
> >>diff --git a/proto/gen_swap_check.py b/proto/gen_swap_check.py
> >>new file mode 100644
> >>index 0000000..f0ca86e
> >>--- /dev/null
> >>+++ b/proto/gen_swap_check.py
> >>@@ -0,0 +1,761 @@
> >>+#!/usr/bin/env python
> >>+import getopt
> >>+import os
> >>+import sys
> >>+import errno
> >>+import re
> >>+
> >>+def _i():
> >>+ '''
> >>+ Indent function
> >>+
> >>+ Returns joined _indent to be concatenated with code string in _c()
> >>+ '''
> >>+ return ''.join(_indent)
> >>+
> >>+# Helping functions and definitions copy-pasted from c_client.py
> >>+
> >>+def _h(fmt, *args):
> >>+ '''
> >>+ Writes the given line to the header file.
> >>+ '''
> >>+ _hlines[_hlevel].append(fmt % args)
> >>+
> >>+def _c(fmt, *args):
> >>+ '''
> >>+ Writes the given line to the source file.
> >>+ '''
> >>+ _clines[_clevel].append(fmt % args)
> >>+
> >>+def _hc(fmt, *args):
> >>+ '''
> >>+ Writes the given line to both the header and source files.
> >>+ '''
> >>+ _h(fmt, *args)
> >>+ _c(fmt, *args)
> >>+
> >>+def _code(fmt, *args):
> >>+ global _codelines
> >>+ _codelines.append(fmt % args)
> >>+
> >>+def output_code():
> >>+ global _codelines
> >>+ for line in _codelines:
> >>+ _c("%s", line)
> >>+ _codelines = []
> >>+
> >>+# Some hacks to make the API more readable and to keep backwards compability
> >>+_cname_special_cases = {'DECnet':'decnet'}
> >>+_extension_special_cases = ['XPrint', 'XCMisc', 'BigRequests']
> >>+
> >>+# The regex matches three types of strings:
> >>+# 1. Those starting with one and only one upper-case letter or a digit
> >>+# and proceeding with lower-case letters, no upper-case letters or
> >>+# digits are allowed except for the first letter
> >>+# 2. Those staring and proceeding with upper-case letters or digits and
> >>+# containing no lower-case letters at all
> >>+# 3. Those starting and proceeding with lower-case letters and containing
> >>+# no upper-case letters or digits at all
> >>+_cname_re = re.compile('([A-Z0-9][a-z]+|[A-Z0-9]+(?![a-z])|[a-z]+)')
> >>+
> >>+_hlines = []
> >>+_hlevel = 0
> >>+_clines = []
> >>+_clevel = 0
> >>+_codelines = []
> >>+
> >>+_ns = None
> >>+_filename = ''
> >>+_requests = {}
> >>+_indent = []
> >>+
> >>+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):
> >>+ '''
> >>+ Does C-name conversion on a single string fragment.
> >>+ Uses a regexp with some hard-coded special cases.
> >>+ '''
> >>+ if str in _cname_special_cases:
> >>+ return _cname_special_cases[str]
> >>+ else:
> >>+ split = _cname_re.finditer(str)
> >>+ name_parts = [match.group(0) for match in split]
> >>+ return '_'.join(name_parts)
> >>+
> >>+def _ext(str):
> >>+ '''
> >>+ Does C-name conversion on an extension name.
> >>+ Has some additional special cases on top of _n_item.
> >>+ '''
> >>+ if str in _extension_special_cases:
> >>+ return _n_item(str).lower()
> >>+ else:
> >>+ return str.lower()
> >>+
> >>+def _n(list):
> >>+ '''
> >>+ Does C-name conversion on a tuple of strings.
> >>+ Different behavior depending on length of tuple, extension/not extension, etc.
> >>+ Basically C-name converts the individual pieces, then joins with underscores.
> >>+ '''
> >>+ if len(list) == 1:
> >>+ parts = list
> >>+ elif len(list) == 2:
> >>+ parts = [list[0], _n_item(list[1])]
> >>+ elif _ns.is_ext:
> >>+ parts = [list[0], _ext(list[1])] + [_n_item(i) for i in list[2:]]
> >>+ else:
> >>+ parts = [list[0]] + [_n_item(i) for i in list[1:]]
> >>+ return '_'.join(parts).lower()
> >>+
> >>+# Copy-pasted from c_client.py code ends here
> >>+
> >>+# Check for the argument that specifies path to the xcbgen python package.
> >>+try:
> >>+ opts, args = getopt.getopt(sys.argv[1:], 'd:p:')
> >>+except getopt.GetoptError as err:
> >>+ print(err)
> >>+ print('Usage: gen_swap_check.py -d generated_dir [-p path] file.xml')
> >>+ sys.exit(1)
> >>+
> >>+for (opt, arg) in opts:
> >>+ if opt == '-d':
> >>+ gendir = arg
> >>+ if opt == '-p':
> >>+ sys.path.insert(1, arg)
> >>+
> >>+def c_open(self):
> >>+ '''
> >>+ Exported function that handles module open.
> >>+ Opens the files and writes out the auto-generated comment,
> >>+ header file includes, etc.
> >>+ '''
> >>+ global _ns
> >>+ _ns = self.namespace
> >>+ _ns.c_ext_global_name = _n(_ns.prefix + ('id',))
> >>+
> >>+ global _filename
> >>+ _filename = ''.join(('swapcheck_',_ns.header))
> >>+
> >>+ _h_setlevel(0)
> >>+ _c_setlevel(0)
> >>+
> >>+ _hc('/*')
> >>+ _hc(' * This file generated automatically from %s by gen_swap_check.py.', _ns.file)
> >>+ _hc(' * Edit at your peril.')
> >>+ _hc(' */')
> >>+ _hc('')
> >>+
> >>+ _h('/**')
> >>+ _h(' * @defgroup XCB_%s_API XCB %s API', _ns.ext_name, _ns.ext_name)
> >>+ _h(' * @brief %s XCB Protocol Implementation.', _ns.ext_name)
> >>+ _h(' * @{')
> >>+ _h(' **/')
> >>+ _h('')
> >>+ _h('#ifndef __%s_H', _ns.header.upper())
> >>+ _h('#define __%s_H', _ns.header.upper())
> >>+ _h('')
> >>+
> >>+ # swapcheck_xproto.h is included in all the others' extensions header files, so
> >>+ # it's very convenient to include the common libs into this header
> >>+ if _ns.header == 'xproto':
> >>+ _h('#include "xorg/misc.h"')
> >>+ _h('#include "X11/X.h"')
> >>+ _h('#include "X11/Xproto.h"')
> >>+ else:
> >>+ _hc('#include "swapcheck_xproto.h"')
> >>+
> >>+ _c('#include "%s.h"', _filename)
> >>+ _c('#include <stdlib.h>')
> >>+ _c('#include <assert.h>')
> >>+ _c('#include <stddef.h> /* for offsetof() */')
> >>+ _c('#include <errno.h>')
> >>+
> >>+ _c('')
> >>+ _c('#define ALIGNOF(type) offsetof(struct { char dummy; type member; }, member)')
> >>+
> >>+ _h('')
> >>+ _h('#ifdef __cplusplus')
> >>+ _h('extern "C" {')
> >>+ _h('#endif')
> >>+
> >>+ if _ns.is_ext:
> >>+ _h('')
> >>+ _h('#define XCB_%s_MAJOR_VERSION %s', _ns.ext_name.upper(), _ns.major_version)
> >>+ _h('#define XCB_%s_MINOR_VERSION %s', _ns.ext_name.upper(), _ns.minor_version)
> >>+ _h('') #XXX
> >>+ #_h('extern xcb_extension_t %s;', _ns.c_ext_global_name)
> >>+
> >>+ _c('')
> >>+ #_c('xcb_extension_t %s = { "%s", 0 };', _ns.c_ext_global_name, _ns.ext_xname)
> >>+
> >>+ _hc('')
> >>+
> >>+def c_close(self):
> >>+ '''
> >>+ Exported function that handles module close.
> >>+ Writes out all the stored content lines, then closes the files.
> >>+ '''
> >>+ _c_setlevel(0)
> >>+ for reqnum in _requests:
> >>+ _c('#define %s %s', 'GEN_' + _n(_requests[reqnum]).upper(), reqnum)
> >>+ _c('')
> >>+
> >>+ _h_setlevel(2)
> >>+ _c_setlevel(2)
> >>+ _hc('')
> >>+
> >>+ for kind in swapCheckDescendants:
> >>+ generate_dispatch(kind(RequestHandler()), self.namespace.header)
> >>+
> >>+ _h('')
> >>+ _h('#ifdef __cplusplus')
> >>+ _h('}')
> >>+ _h('#endif')
> >>+
> >>+ _h('')
> >>+ _h('#endif')
> >>+ _h('')
> >>+ _h('/**')
> >>+ _h(' * @}')
> >>+ _h(' */')
> >>+
> >>+ # Ensure the gen subdirectory exists
> >>+ try:
> >>+ os.mkdir(gendir)
> >>+ except OSError as e:
> >>+ if e.errno != errno.EEXIST:
> >>+ raise
> >>+
> >>+ # Write header file
> >>+ hfile = open('%s/%s.h' % (gendir, _filename), 'w')
> >>+ for list in _hlines:
> >>+ for line in list:
> >>+ hfile.write(line)
> >>+ hfile.write('\n')
> >>+ hfile.close()
> >>+
> >>+ # Write source file
> >>+ cfile = open('%s/%s.c' % (gendir, _filename), 'w')
> >>+ for list in _clines:
> >>+ for line in list:
> >>+ cfile.write(line)
> >>+ cfile.write('\n')
> >>+
> >>+ cfile.close()
> >>+
> >>+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.
> >>+ '''
> >>+ #Needs future implementation
> >>+
> >>+
> >>+def c_enum(self, name):
> >>+ '''
> >>+ Exported function that handles enum declarations.
> >>+
> >>+ Private fields:
> >>+ * fields dictonary contains (enum_number -> enum_name) pair, which
> >>+ represents enum entry
> >>+ '''
> >>+ _fields = {}
> >>+
> >>+ if self.values:
> >>+ for entry in self.values:
> >>+ _fields[entry[1]] = entry[0]
> >>+ else: #if self.bits
> >>+ for entry in self.bits:
> >>+ _fields[2**entry[1]] = entry[0] # dunno if it's right
> >>+
> >>+ _c('%stypedef enum %s {', _i(), _n(name))
> >>+ _increase_indent()
> >>+ for enum_num in _fields:
> >>+ _c('%s%s = %s,', _i(), (_n(name)+'_'+_fields[enum_num]).upper(), enum_num)
> >>+
> >>+ _decrease_indent()
> >>+ _c('%s} %s_t;', _i(), _n(name))
> >>+ _c('')
> >>+
> >>+def c_struct(self, name):
> >>+ '''
> >>+ Exported function that handles struct declarations.
> >>+ '''
> >>+ _h_setlevel(1)
> >>+ _c_setlevel(1)
> >>+
> >>+ for kind in swapCheckDescendants:
> >>+ _swapcheck = kind(StructHandler())
> >>+ _swapcheck.generate(self, name)
> >>+
> >>+def c_union(self, name):
> >>+ '''
> >>+ Exported function that handles union declarations (union is deprecated)
> >>+ '''
> >>+ #Needs future implementation
> >>+
> >>+class SwapCheck:
> >>+ '''
> >>+ Represent abstract swapper/checker, that generates appropriate c-code
> >>+
> >>+ Created combined because these classes share a lot functions
> >>+ _name is hardcoded literal that represents name of it's class, is used to concatenate it with other stuff to generate functions
> >>+ _docheck is bool that determines accurate position to define afterEnd by calling determine_afterEnd
> >this should be linewrapped. I'm also struggling - where is the copied code
> >and where is the new code? again, this should be marked with comments to
> >help focusing on the actual review
> The copied code begins starts with "
>
> # Helping functions and definitions copy-pasted from c_client.py
>
> " comment, and ends with "
>
> # Copy-pasted from c_client.py code ends here
>
oh, right. I read over that. maybe you can make this more obvious in the
form of
###############################################################
# Copy-pasted from c_client.py code ends here
###############################################################
That sticks out a lot more.
> "
> Could you please clarify what is "linewrapped"? Wrapped with comment? Or
> indents? Or else?
linewrapped as in: the width should be max of 80 chars, so this line
becomes:
_name is hardcoded literal that represents name of it's class, is
used to concatenate it with other stuff to generate functions
(I don't know what indentation requirements pydoc has)
>
> >>+ '''
> >>+ _name = None
> >>+ _docheck = False
> >>+ _reusedvars = []
> >>+ _declaredvars = []
> >>+ _typeHandler = None
> >>+ _has_struct = False
> >>+
> >>+ def __init__(self, typeHandler):
> >>+ self._typeHandler = typeHandler
> >>+
> >>+ def access_check( self, size ):
> >>+ if self._docheck:
> >>+ _code('%sif( p + %u > (uint8_t*)afterEnd) {', _i(), size)
> >>+ _increase_indent()
> >>+ _code('%sreturn BadLength;', _i())
> >>+ _decrease_indent()
> >>+ _code('%s}', _i())
> >general nitpick: it's usually better to invert a condition and
> >return/break/continue early than have the whole block indented. so in this
> >case you'd do a
> > if not self._docheck:
> > return
> >
> > .... other code
> Fixed.
> >>+
> >>+ def process_fieldvalue(self, fname, ftype):
> >>+ self._docheck = self._typeHandler.determine_afterEnd(self._docheck, fname)
> >>+ if fname in self._reusedvars:
> >>+ _varname = 'fieldvalue' + '_' + ''.join(fname)
> >>+ _datatype = _n(ftype.name)
> >>+ if(fname not in self._declaredvars):
> >>+ _c('%s%s %s;', _i(), _datatype, _varname)
> >>+ self._declaredvars.append(fname)
> >>+ _code('%s%s = *(%s*)p;', _i(), _varname, _datatype)
> >different indentation to the rest of the code, and the above comment applies
> >here too
> >
> >>+ def funcName(self, name):
> >>+ return '_'.join(name) + '_' + self.nameToString()
> >>+
> >>+ def printHeader(self, type):
> >>+ _hc('int')
> >>+ self._typeHandler.printSwapCheckFuncSignature(self.funcName(type.name))
> >>+ _increase_indent()
> >>+ _c('%s//type, field_type, field_name, visible, wire, auto, enum, isfd', _i())
> >>+ _c('')
> >>+ _c('%suint8_t* p = (uint8_t*)data;', _i())
> >>+
> >>+ if self._has_struct:
> >>+ self._typeHandler.initAfterStruct()
> >>+ self._typeHandler.init_afterEnd()
> >>+
> >>+ _c('')
> >>+
> >>+ def printFieldHeader(self, field):
> >>+ _code('%s//%s, %s, %s, %s, %s, %s, %s', _i(),
> >>+ field.field_type,
> >>+ field.field_name,
> >>+ field.visible,
> >>+ field.wire,
> >>+ field.auto,
> >>+ field.enum,
> >>+ field.isfd)
> >whoah, that is a lot of tabs...
> I assumed tab = 4 spaces, as it is defined in
> http://www.x.org/wiki/CodingStyle/. As python strictly requires tabs and not
> space-indents, I thought it would be the nicer way to organize code. But I
> will switch to tab = 8 spaces and fix all the tabs issues.
iirc python only requires the same whitespace everywhere, right? so if you
have spaces everywhere, you need to have spaces. but if the copy-pasted code
uses tabs, stay with that.
but now that you say that, after changing the tabstop to 4 this looks a lot
better. as a general rule: _never_ set your tabstop to 4 spaces. A tab is 8
spaces, ignore anyone telling you anything different ;)
there's a difference between tabstop and indentation, you can have tabstop 8
and indentation 4, so you get things like:
if (....)
if (...)
blah
not recommended. You're generally better off switching to spaces everywhere
or using a 1-tab indent everywhere. Then again, many bikesheds have been
painted during tab vs spaces indentation arguments.
the copied xcb code looks like it has tab-only indent, so the easiest
solution here is to set your tabstop to 8 and done.
given that this is generator code in a different language it's acceptable
that there's a slightly different coding style. make sure the output is
4-space indented and that's it.
Cheers,
Peter
> >>+
> >>+ def process_simple(self, fname, ftype):
> >>+ pass
> >>+
> >>+ def process_list(self, ftype, it, llen):
> >>+ #_c('%s{', _i())
> >>+ if it not in self._declaredvars:
> >>+ _c('%sunsigned int %s;', _i(), it)
> >>+ self._declaredvars.append(it)
> >>+ #_increase_indent()
> >>+ _code('%sfor(%s = 0; %s < %s; %s++)', _i(), it, it, llen, it)
> >>+ _code('%s{', _i())
> >>+
> >>+ _increase_indent()
> >>+ self.check(ftype.member, None)
> >>+ _decrease_indent()
> >>+
> >>+ _code('%s}', _i())
> >>+ #_decrease_indent()
> >>+ #_code('%s}', _i())
> >>+
> >>+ def process_struct(self, tname):
> >>+ self._typeHandler.process_struct(self.funcName(tname))
> >>+
> >>+ def process_switch(self, ftype):
> >>+ _casevarn = 'fieldvalue_' + ftype.expr.lenfield_name # case variable name
> >>+ _code('\n%s//switch begins\n', _i())
> >>+
> >>+ for case in ftype.bitcases:
> >>+ _eq_sign = '==' if case.type.is_case else '&'
> >>+
> >>+ if case.type.expr: # if bitcase/case has enumref
> >>+ _enumn = case.type.expr[0].lenfield_type.name # enum name
> >>+ _enument = case.type.expr[0].lenfield_name # enum entry name #TODO WHAT IF NOT 0?
> >>+ _code('%sif (%s %s %s)', _i(),
> >>+ _casevarn, _eq_sign,
> >>+ (_n(_enumn)+'_'+_enument).upper())
> >>+ _code('%s{', _i())
> >>+ _increase_indent()
> >>+ for field in case.type.fields:
> >>+ _code('%s//%s %s', _i(), field.type.name, field.field_name)
> >>+ self.check(field.type, field.field_name)
> >>+ _decrease_indent()
> >>+ _code('%s}', _i())
> >>+ _code('')
> >>+
> >>+ _code('%s//switch ends\n', _i())
> >>+
> >>+ def check(self, ftype, fname):
> >>+ _size = ftype.size
> >>+ if ftype.is_simple or ftype.is_expr:
> >>+ self.process_simple(fname, ftype)
> >>+ _code('%sp += %u;', _i(), _size)
> >>+ elif ftype.is_pad and ftype.fixed_size():
> >>+ byts = ftype.nmemb
> >>+ _code('%sp += %u;', _i(), byts)
> >>+ elif ftype.is_pad and not ftype.fixed_size():
> >>+ al = ftype.align
> >>+ _code('%sp += %u;', _i(), al)
> >>+ elif ftype.is_list and ftype.fixed_size():
> >>+ self.process_list(ftype, 'i_'+fname, ftype.nmemb)
> >>+ elif ftype.is_list and not ftype.fixed_size():
> >>+ self.process_list(ftype,
> >>+ 'i_'+ftype.expr.lenfield_name,
> >>+ 'fieldvalue_' + ftype.expr.lenfield_name)
> >indentation
> >
> >>+ elif ftype.is_switch:
> >>+ self.process_switch(ftype)
> >>+ elif ftype.is_container:
> >>+ self.process_struct(ftype.name)
> >>+ else:
> >>+ _code( '%s#error yet not implemented', _i())
> >>+
> >>+ def generate(self, item, name):
> >>+ self._docheck = self._typeHandler.init_docheck(self._docheck)
> >>+ _afterEnd = None
> >>+
> >>+ self._reusedvars = self.checkReusedVariables(item, [])
> >>+ self._declaredvars = []
> >>+ for field in item.fields:
> >>+ if self.isAfterStructNedeed(field.type):
> >>+ break
> >>+ self.printHeader(item)
> >>+
> >>+ for field in item.fields:
> >>+ self.printFieldHeader(field)
> >>+ self.check(field.type, field.field_name)
> >>+ self.printEmpty()
> >>+ self._typeHandler.printFooter()
> >>+ self._typeHandler.printReturn()
> >>+ self.directPrintEmpty()
> >>+ output_code()
> >>+
> >>+ def nameToString(self):
> >>+ return self._name
> >>+
> >>+ def isAfterStructNedeed(self, ftype):
> >>+ '''
> >>+ recurce over every field in request to find out whether it contains structs
> >>+
> >>+ to avoid afterEnd is declared but not used warning we iterate over all
> >>+ fields in the request and if it contains a single struct entry we must
> >>+ declare it, otherwise we must not.
> >>+ '''
> >>+ if ftype.is_list:
> >>+ self._has_struct = self.isAfterStructNedeed(ftype.member) # check if members of list are structs
> >>+ elif ftype.is_switch:
> >>+ for sfield in ftype.fields: # check if any field of switch is a struct
> >>+ if self.isAfterStructNedeed(sfield.type):
> >>+ self._has_struct = True
> >>+ elif ftype.is_container:
> >>+ self._has_struct = True
> >>+ else:
> >>+ self._has_struct = False
> >>+ return self._has_struct
> >>+
> >>+ def printEmpty(self):
> >>+ _code('')
> >>+
> >>+ def directPrintEmpty(self):
> >>+ _c('')
> >>+
> >>+ def checkReusedVariables(self, _container, other):
> >>+ appendvars = []
> >>+ listvars = []
> >>+ othervars = other
> >>+ for field in _container.fields:
> >>+ if field.type.is_list or field.type.is_switch:
> >>+ listvars.append(field.type.expr.lenfield_name)
> >>+ if field.type.is_switch:
> >>+ appendvars.extend(self.checkReusedVariables(field.type, othervars))
> >>+ else:
> >>+ othervars.append(field.field_name)
> >>+ listvars = list(set(listvars) & set(othervars))
> >>+ listvars.extend(appendvars)
> >>+ return listvars
> >>+
> >>+class SwapParent(SwapCheck):
> >>+ '''
> >>+ Represents abstract class to generate toClient and fromClient swapping functions
> >>+ '''
> >>+ def process_simple(self, fname, ftype):
> >>+ self.before_swap_simplefield(fname, ftype) #before swap hook
> >>+ self.swap_simplefield(ftype.size)
> >>+ self.after_swap_simplefield(fname, ftype) #after swap hook
> >>+
> >>+ def swap_simplefield(self, _size ):
> >>+ if _size == 1:
> >>+ pass
> >>+ elif _size == 2:
> >>+ self.access_check(_size )
> >>+ self.swap_with_swapper('swaps', 'uint16_t')
> >>+ elif _size ==4 :
> >>+ self.access_check( _size )
> >>+ self.swap_with_swapper('swapl', 'uint32_t')
> >>+ else:
> >>+ _c( ' #error swap of size %d not implemented', _size )
> >>+
> >>+ def swap_with_swapper( self, swapper, size ):
> >>+ _code('%s%s((%s*)p);', _i(), swapper, size)
> >>+
> >>+ def before_swap_simplefield( self, fname, ftype ):
> >>+ pass
> >>+
> >>+ def after_swap_simplefield( self, fname, ftype ):
> >>+ pass
> >>+
> >>+class SwapFromClient(SwapParent):
> >>+ _name = 'SwapFromClient'
> >>+
> >>+ def after_swap_simplefield( self, fname, ftype):
> >>+ self.process_fieldvalue( fname, ftype )
> >>+
> >>+class SwapToClient(SwapParent):
> >>+ _name = 'SwapToClient'
> >>+
> >>+ def before_swap_simplefield( self, fname, ftype):
> >>+ self.process_fieldvalue( fname, ftype)
> >>+
> >>+class Check(SwapCheck):
> >>+ _name = 'Check'
> >>+
> >>+ def process_simple(self, fname, _size):
> >>+ self.process_fieldvalue(fname, _size)
> >>+
> >>+class TypeHandler:
> >>+ def printFooter(self):
> >>+ pass
> >>+
> >>+ def determine_afterEnd(self, _docheck, fname):
> >>+ pass
> >>+
> >>+ def printReturn(self):
> >whitespace issue here
> Fixed.
> >>+ _code(' return Success;')
> >>+ _code(' else')
> >>+ _code(' return BadLength;')
> >>+ _code('}')
> >>+ _decrease_indent()
> >>+ _h('')
> >>+ _code('')
> >>+
> >>+ def printSwapCheckFuncSignature(self, name):
> >>+ pass
> >>+
> >>+ def init_afterEnd(self):
> >>+ pass
> >>+
> >>+ def init_docheck(self, docheck):
> >>+ pass
> >>+
> >>+ def initAfterStruct(self):
> >>+ pass
> >>+
> >>+ def process_struct(self, name):
> >>+ pass
> >>+
> >>+class RequestHandler(TypeHandler):
> >>+ def printFooter(self):
> >>+ _code(' if (p == afterEnd)')
> >>+
> >>+ def determine_afterEnd(self, _docheck, fname):
> >>+ """
> >>+ Overloaded function to return the appropriate value of _docheck,
> >>+
> >>+ unlike the StructHandler does
> >>+ """
> >>+ if fname == 'length':
> >>+ _code(' afterEnd = ((uint8_t*)data) + 4 * ( *(uint16_t*)p );')
> >>+ return True
> >>+ else:
> >>+ return _docheck
> >>+ def printSwapCheckFuncSignature(self, name):
> >>+ _h('%s(void *data);', name)
> >>+ _c('%s(void *data)\n{', name)
> >>+
> >>+ def init_afterEnd(self):
> >>+ _c(' uint8_t* afterEnd = NULL;')
> >shouldn't you be using _i() here instead of the tab? in the generated code
> >Chris sent late Feb this line is always over-indented
> Fixed
> >>+
> >>+ def init_docheck(self, docheck):
> >>+ return False
> >>+
> >>+ def initAfterStruct(self):
> >>+ _c(' uint8_t* afterStruct = NULL;')
> >same here
> >
> >>+
> >>+ def process_struct(self, name):
> >>+ _code('%sif(%s(p, afterEnd, &afterStruct) != Success)\n\t\t\treturn BadLength;', _i(), name)
> >you need to use _i() consistently here, and best to have multiple print
> >lines. otherwise the indentation is messed up
> Fixed
> >>+ _code('%sp = afterStruct;', _i())
> >>+
> >>+class StructHandler(TypeHandler):
> >>+ def printFooter(self):
> >>+ _code(' *afterStruct = p;')
> >>+ _code(' if (p <= (uint8_t*)afterEnd)')
> >same here
> Fixed
> >afaict the output is correct, so this looks good. going through the final
> >patch with a fine-tooth-comb will be interesting. Have you run the X test
> >suite against this by any chance? That's probably the most reliable check to
> >see if something is buggy.
> I haven't run the X test suit, but so far even running the gnome-terminal
> fails, though xeyes and xterm work OK, both with and without byte-swapping.
> I am working on gnome-terminal BadLength bug these days. I will post a patch
> after clarifying the questions above, so it fully covers the topics of this
> letter
> >Cheers,
> > Peter
> >
> >>+
> >>+ def printSwapCheckFuncSignature(self, name):
> >>+ _h('%s(void *data, void *afterEnd, uint8_t **afterStruct);', name)
> >>+ _c('%s(void *data, void *afterEnd, uint8_t **afterStruct)\n{', name)
> >>+
> >>+ def init_docheck(self, docheck):
> >>+ return True
> >>+
> >>+ def process_struct(self, name):
> >>+ _code('%sif(%s(p, afterEnd, afterStruct) != Success)\n\t\t\treturn BadLength;', _i(), name)
> >>+ _code('%sp = (uint8_t*)(*afterStruct);', _i())
> >>+
> >>+ def determine_afterEnd(self, _docheck, fname):
> >>+ """
> >>+ Overloaded function to return the same value as _docheck had,
> >>+
> >>+ unlike the RequestHandler's one
> >>+ """
> >>+ return _docheck
> >>+
> >>+def generate_dispatch(kind, name):
> >>+ """
> >>+ Function to generate dispatch function
> >>+
> >>+ kind is an object of class SwapCheck
> >>+ name is the name of extension
> >>+ """
> >>+ #print function header-signature
> >>+ _hc('%sint', _i())
> >>+ _h('%sxcb_%s_dispatch(void *req);', _i(), name+'_'+kind._name)
> >>+ _c('%sxcb_%s_dispatch(void *req)\n{', _i(), name+'_'+kind._name)
> >>+ _increase_indent()
> >>+
> >>+ # init variables according to c90 std
> >>+ _c('%slong switch_item;', _i())
> >>+ _c('%sint return_val = 0;', _i())
> >>+ _c('%sxReq* request = (xReq*)req;', _i())
> >>+
> >>+ # define varible to switch, depends on extension
> >>+ _switch_item = 'request->'
> >>+ _switch_item = _switch_item + 'reqType' if name == 'xproto' else _switch_item + 'data'
> >>+ _c('%sswitch_item = %s;', _i(), _switch_item)
> >>+
> >>+ # print switch header
> >>+ _c('%sswitch(switch_item)', _i())
> >>+ _c('%s{', _i())
> >>+ _increase_indent()
> >>+
> >>+ # for every opcode in generated (opcode -> name) dict "_requests"
> >>+ # print case
> >>+ for reqnum in _requests:
> >>+ _c('%scase %s:', _i(), 'GEN_'+_n(_requests[reqnum]).upper())
> >>+ _increase_indent()
> >>+ _c('%sreturn_val = %s((void*)request);', _i(),
> >>+ kind.funcName(_requests[reqnum]))
> >>+ _c('%sbreak;', _i())
> >>+ _decrease_indent()
> >>+ _c('%sdefault:', _i())
> >>+ _increase_indent()
> >>+ _c('%sreturn BadRequest;', _i())
> >>+ _decrease_indent()
> >>+
> >>+ _c('%s}', _i())
> >>+ _decrease_indent()
> >>+ _c('%sreturn return_val;', _i())
> >>+ _decrease_indent()
> >>+ _c('%s}', _i())
> >>+
> >>+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}
> >>+
> >>+def c_event(self, name):
> >>+ '''
> >>+ Exported function that handles event declarations.
> >>+ '''
> >>+ #Needs future implementation
> >>+
> >>+def c_error(self, name):
> >>+ '''
> >>+ Exported function that handles error declarations.
> >>+ '''
> >>+ #Needs future implementation
> >>+
> >>+# 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,
> >>+ }
> >>+
> >>+# 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()
> >>--
> >>1.9.1
> >>
>
More information about the xorg-devel
mailing list