[prev in list] [next in list] [prev in thread] [next in thread]
List: freedesktop-xcb
Subject: Re: [Xcb] [PATCH libxcb 11/18] c_client.py: don't generate useless empty /** < */ comments
From: Christian Linhart <chris () DemoRecorder ! com>
Date: 2015-03-15 10:47:51
Message-ID: 55056357.5070007 () DemoRecorder ! com
[Download RAW message or body]
Hi Ran,
Can you please regenerate this patch so it works with the current HEAD of the repo?
Thank you,
Chris
P.S.: If you don't have time, please tell me and I'll make a new patch.
On 10/29/14 08:33, Christian Linhart wrote:
> On 10/14/14 13:51, Ran Benita wrote:
> > Hi Chris, thanks for reviewing!
> >
> > On Mon, Oct 13, 2014 at 05:50:35PM +0200, Christian Linhart wrote:
> > > Some of these changes will be lots of work to merge with my pending changes.
> > > (trivial work, but lots of it)
> > >
> > > Therefore I suggest to skip this patch for now,
> > > and recreate it after my changes have been merged to upstream.
> > Sure, any of these can be skipped if they cause trouble.
> Good, thank you.
>
> When I review your patches, I keep an eye on possible merge conflicts.
> Your other changes that I have reviewed so far, look good in this respect.
>
> So far, only this change causes trouble.
>
> > Easy conflicts
> > though can be readily resolved by the one doing the merging.
> Yep.
>
> But we have to be careful because merging is always a risk to introduce new bugs.
>
> I have some special and painful experience about that from a project with a big \
> source-code base:
> At some point it was decided to create a lot of branches because of some perceived \
> advantages for QA.
> The result was a lot of merging of course.
>
> And it turned out that merging causes at least the following two types of bugs:
> 1. Bugs which are created by human error, i.e., when manually resolving merge \
> conflicts. 2. Bugs which are created automatically:
> This can happen when there is no textual merge conflict, but a semantic merge \
> conflict. In that case, the merge tool automatically performs the textual merge \
> without flagging that as a conflict.
> Therefore, merging can create a code-mixture that was neither written by a human
> not read by a human, and that does not make any sense.
> But the compiler may still accept it.
>
> Of course, bugs of type 2 are harder to avoid.
>
> To mitigate the issue we made it mandatory to review all merges,
> including the code which showed no textual merge conflicts.
>
> As you can imagine, the productivity of the development department
> dropped very significantly to the point where development
> essentially came to a grinding halt.
>
> Result: change of policy to minimize the need of merging.
> Almost following a mantra like "Merging considered harmful".
>
> ***
>
> That's why I am reviewing your patches for merge-conflicts before the actual merge \
> happens.
> And that's also why I wait for all pending patches merged
> into upstream before I'll post any new changes.
> (some of which I have already implemented.
> And I am using them already in the temporary private fork of XCB which I use in my \
> product.)
> Chris
>
> > > Recreating this patch should be easy because it looks like global replace in \
> > > the editor.
> > Yep.
> Good.
>
> I have written that on my TODO-list,
> so that we'll think about recreating this patch
> after we'll have merged all pending patches to upstream.
>
> Chris
>
> > Ran
> >
> > > Chris
> > >
> > > On 10/12/14 20:58, Ran Benita wrote:
> > > > (This does not change doxygen's output or warnings).
> > > >
> > > > Signed-off-by: Ran Benita <ran234@gmail.com>
> > > > ---
> > > > src/c_client.py | 88 \
> > > > ++++++++++++++++++++++++++++----------------------------- 1 file changed, 44 \
> > > > insertions(+), 44 deletions(-)
> > > > diff --git a/src/c_client.py b/src/c_client.py
> > > > index 4c19722..c1839c7 100644
> > > > --- a/src/c_client.py
> > > > +++ b/src/c_client.py
> > > > @@ -1111,7 +1111,7 @@ def _c_serialize(context, self):
> > > > for p in params:
> > > > typespec, pointerspec, field_name = p
> > > > spacing = ' '*(maxtypelen-len(typespec)-len(pointerspec))
> > > > - param_str.append("%s%s%s %s%s /**< */" % (indent, typespec, \
> > > > spacing, pointerspec, field_name)) + param_str.append("%s%s%s %s%s" % \
> > > > (indent, typespec, spacing, pointerspec, field_name)) # insert function name
> > > > param_str[0] = "%s (%s" % (func_name, param_str[0].strip())
> > > > param_str = ["%s," % x for x in param_str]
> > > > @@ -1292,9 +1292,9 @@ def _c_iterator(self, name):
> > > > _h(' * @brief %s', self.c_iterator_type)
> > > > _h(' **/')
> > > > _h('typedef struct %s {', self.c_iterator_type)
> > > > - _h(' %s *data; /**< */', self.c_type)
> > > > - _h(' int%s rem; /**< */', ' ' * (len(self.c_type) - 2))
> > > > - _h(' int%s index; /**< */', ' ' * (len(self.c_type) - 2))
> > > > + _h(' %s *data;', self.c_type)
> > > > + _h(' int%s rem;', ' ' * (len(self.c_type) - 2))
> > > > + _h(' int%s index;', ' ' * (len(self.c_type) - 2))
> > > > _h('} %s;', self.c_iterator_type)
> > > >
> > > > _h_setlevel(1)
> > > > @@ -1310,8 +1310,8 @@ def _c_iterator(self, name):
> > > > _h(' */')
> > > > _c('')
> > > > _hc('void')
> > > > - _h('%s (%s *i /**< */);', self.c_next_name, self.c_iterator_type)
> > > > - _c('%s (%s *i /**< */)', self.c_next_name, self.c_iterator_type)
> > > > + _h('%s (%s *i);', self.c_next_name, self.c_iterator_type)
> > > > + _c('%s (%s *i)', self.c_next_name, self.c_iterator_type)
> > > > _c('{')
> > > >
> > > > if not self.fixed_size():
> > > > @@ -1351,8 +1351,8 @@ def _c_iterator(self, name):
> > > > _h(' */')
> > > > _c('')
> > > > _hc('xcb_generic_iterator_t')
> > > > - _h('%s (%s i /**< */);', self.c_end_name, self.c_iterator_type)
> > > > - _c('%s (%s i /**< */)', self.c_end_name, self.c_iterator_type)
> > > > + _h('%s (%s i);', self.c_end_name, self.c_iterator_type)
> > > > + _c('%s (%s i)', self.c_end_name, self.c_iterator_type)
> > > > _c('{')
> > > > _c(' xcb_generic_iterator_t ret;')
> > > >
> > > > @@ -1457,8 +1457,8 @@ def _c_accessors_field(self, field):
> > > > if field.type.is_simple:
> > > > _hc('')
> > > > _hc('%s', field.c_field_type)
> > > > - _h('%s (const %s *R /**< */);', field.c_accessor_name, c_type)
> > > > - _c('%s (const %s *R /**< */)', field.c_accessor_name, c_type)
> > > > + _h('%s (const %s *R);', field.c_accessor_name, c_type)
> > > > + _c('%s (const %s *R)', field.c_accessor_name, c_type)
> > > > _c('{')
> > > > if field.prev_varsized_field is None:
> > > > _c(' return (%s *) (R + 1);', field.c_field_type)
> > > > @@ -1475,8 +1475,8 @@ def _c_accessors_field(self, field):
> > > > return_type = '%s *' % field.c_field_type
> > > >
> > > > _hc(return_type)
> > > > - _h('%s (const %s *R /**< */);', field.c_accessor_name, c_type)
> > > > - _c('%s (const %s *R /**< */)', field.c_accessor_name, c_type)
> > > > + _h('%s (const %s *R);', field.c_accessor_name, c_type)
> > > > + _c('%s (const %s *R)', field.c_accessor_name, c_type)
> > > > _c('{')
> > > > if field.prev_varsized_field is None:
> > > > _c(' return (%s) (R + 1);', return_type)
> > > > @@ -1562,8 +1562,8 @@ def _c_accessors_list(self, field):
> > > > _hc('')
> > > > _hc('%s *', field.c_field_type)
> > > >
> > > > - _h('%s (%s /**< */);', field.c_accessor_name, params[idx][0])
> > > > - _c('%s (%s /**< */)', field.c_accessor_name, params[idx][0])
> > > > + _h('%s (%s);', field.c_accessor_name, params[idx][0])
> > > > + _c('%s (%s)', field.c_accessor_name, params[idx][0])
> > > >
> > > > _c('{')
> > > > if switch_obj is not None:
> > > > @@ -1586,14 +1586,14 @@ def _c_accessors_list(self, field):
> > > > _hc('')
> > > > _hc('int')
> > > > if switch_obj is not None:
> > > > - _hc('%s (const %s *R /**< */,', field.c_length_name, R_obj.c_type)
> > > > + _hc('%s (const %s *R,', field.c_length_name, R_obj.c_type)
> > > > spacing = ' '*(len(field.c_length_name)+2)
> > > > - _h('%sconst %s *S /**< */);', spacing, S_obj.c_type)
> > > > - _c('%sconst %s *S /**< */)', spacing, S_obj.c_type)
> > > > + _h('%sconst %s *S);', spacing, S_obj.c_type)
> > > > + _c('%sconst %s *S)', spacing, S_obj.c_type)
> > > > length = _c_accessor_get_expr(field.type.expr, fields)
> > > > else:
> > > > - _h('%s (const %s *R /**< */);', field.c_length_name, c_type)
> > > > - _c('%s (const %s *R /**< */)', field.c_length_name, c_type)
> > > > + _h('%s (const %s *R);', field.c_length_name, c_type)
> > > > + _c('%s (const %s *R)', field.c_length_name, c_type)
> > > > length = _c_accessor_get_expr(field.type.expr, fields)
> > > > _c('{')
> > > > _c(' return %s;', length)
> > > > @@ -1603,13 +1603,13 @@ def _c_accessors_list(self, field):
> > > > _hc('')
> > > > _hc('xcb_generic_iterator_t')
> > > > if switch_obj is not None:
> > > > - _hc('%s (const %s *R /**< */,', field.c_end_name, R_obj.c_type)
> > > > + _hc('%s (const %s *R,', field.c_end_name, R_obj.c_type)
> > > > spacing = ' '*(len(field.c_end_name)+2)
> > > > - _h('%sconst %s *S /**< */);', spacing, S_obj.c_type)
> > > > - _c('%sconst %s *S /**< */)', spacing, S_obj.c_type)
> > > > + _h('%sconst %s *S);', spacing, S_obj.c_type)
> > > > + _c('%sconst %s *S)', spacing, S_obj.c_type)
> > > > else:
> > > > - _h('%s (const %s *R /**< */);', field.c_end_name, c_type)
> > > > - _c('%s (const %s *R /**< */)', field.c_end_name, c_type)
> > > > + _h('%s (const %s *R);', field.c_end_name, c_type)
> > > > + _c('%s (const %s *R)', field.c_end_name, c_type)
> > > > _c('{')
> > > > _c(' xcb_generic_iterator_t i;')
> > > >
> > > > @@ -1635,13 +1635,13 @@ def _c_accessors_list(self, field):
> > > > _hc('')
> > > > _hc('%s', field.c_iterator_type)
> > > > if switch_obj is not None:
> > > > - _hc('%s (const %s *R /**< */,', field.c_iterator_name, \
> > > > R_obj.c_type) + _hc('%s (const %s *R,', field.c_iterator_name, \
> > > > R_obj.c_type) spacing = ' '*(len(field.c_iterator_name)+2)
> > > > - _h('%sconst %s *S /**< */);', spacing, S_obj.c_type)
> > > > - _c('%sconst %s *S /**< */)', spacing, S_obj.c_type)
> > > > + _h('%sconst %s *S);', spacing, S_obj.c_type)
> > > > + _c('%sconst %s *S)', spacing, S_obj.c_type)
> > > > else:
> > > > - _h('%s (const %s *R /**< */);', field.c_iterator_name, c_type)
> > > > - _c('%s (const %s *R /**< */)', field.c_iterator_name, c_type)
> > > > + _h('%s (const %s *R);', field.c_iterator_name, c_type)
> > > > + _c('%s (const %s *R)', field.c_iterator_name, c_type)
> > > > _c('{')
> > > > _c(' %s i;', field.c_iterator_type)
> > > >
> > > > @@ -1736,10 +1736,10 @@ def _c_complex(self, force_packed = False):
> > > > # necessary for unserialize to work
> > > > (self.is_switch and field.type.is_switch)):
> > > > spacing = ' ' * (maxtypelen - len(field.c_field_type))
> > > > - _h('%s %s%s %s%s; /**< */', space, field.c_field_type, \
> > > > spacing, field.c_field_name, field.c_subscript) + _h('%s %s%s \
> > > > %s%s;', space, field.c_field_type, spacing, field.c_field_name, \
> > > > field.c_subscript) else:
> > > > spacing = ' ' * (maxtypelen - (len(field.c_field_type) + 1))
> > > > - _h('%s %s%s *%s%s; /**< */', space, field.c_field_type, \
> > > > spacing, field.c_field_name, field.c_subscript) + _h('%s %s%s \
> > > > *%s%s;', space, field.c_field_type, spacing, field.c_field_name, \
> > > > field.c_subscript)
> > > > if not self.is_switch:
> > > > for field in struct_fields:
> > > > @@ -1916,9 +1916,9 @@ def _c_request_helper(self, name, cookie_type, void, \
> > > > regular, aux=False, reply_f
> > > > spacing = ' ' * (maxtypelen - len('xcb_connection_t'))
> > > > comma = ',' if len(param_fields) else ');'
> > > > - _h('%s (xcb_connection_t%s *c /**< */%s', func_name, spacing, comma)
> > > > + _h('%s (xcb_connection_t%s *c%s', func_name, spacing, comma)
> > > > comma = ',' if len(param_fields) else ')'
> > > > - _c('%s (xcb_connection_t%s *c /**< */%s', func_name, spacing, comma)
> > > > + _c('%s (xcb_connection_t%s *c%s', func_name, spacing, comma)
> > > >
> > > > func_spacing = ' ' * (len(func_name) + 2)
> > > > count = len(param_fields)
> > > > @@ -1931,10 +1931,10 @@ def _c_request_helper(self, name, cookie_type, void, \
> > > > regular, aux=False, reply_f c_pointer = '*'
> > > > spacing = ' ' * (maxtypelen - len(c_field_const_type))
> > > > comma = ',' if count else ');'
> > > > - _h('%s%s%s %s%s /**< */%s', func_spacing, c_field_const_type,
> > > > + _h('%s%s%s %s%s%s', func_spacing, c_field_const_type,
> > > > spacing, c_pointer, field.c_field_name, comma)
> > > > comma = ',' if count else ')'
> > > > - _c('%s%s%s %s%s /**< */%s', func_spacing, c_field_const_type,
> > > > + _c('%s%s%s %s%s%s', func_spacing, c_field_const_type,
> > > > spacing, c_pointer, field.c_field_name, comma)
> > > >
> > > > count = 2
> > > > @@ -2132,10 +2132,10 @@ def _c_reply(self, name):
> > > > _h(' */')
> > > > _c('')
> > > > _hc('%s *', self.c_reply_type)
> > > > - _hc('%s (xcb_connection_t%s *c /**< */,', self.c_reply_name, spacing1)
> > > > - _hc('%s%s cookie /**< */,', spacing3, self.c_cookie_type)
> > > > - _h('%sxcb_generic_error_t%s **e /**< */);', spacing3, spacing2)
> > > > - _c('%sxcb_generic_error_t%s **e /**< */)', spacing3, spacing2)
> > > > + _hc('%s (xcb_connection_t%s *c,', self.c_reply_name, spacing1)
> > > > + _hc('%s%s cookie,', spacing3, self.c_cookie_type)
> > > > + _h('%sxcb_generic_error_t%s **e);', spacing3, spacing2)
> > > > + _c('%sxcb_generic_error_t%s **e)', spacing3, spacing2)
> > > > _c('{')
> > > >
> > > > if len(unserialize_fields)>0:
> > > > @@ -2192,9 +2192,9 @@ def _c_reply_fds(self, name):
> > > > _h(' */')
> > > > _c('')
> > > > _hc('int *')
> > > > - _hc('%s (xcb_connection_t%s *c /**< */,', self.c_reply_fds_name, \
> > > > spacing1)
> > > > - _h('%s%s *reply /**< */);', spacing3, self.c_reply_type)
> > > > - _c('%s%s *reply /**< */)', spacing3, self.c_reply_type)
> > > > + _hc('%s (xcb_connection_t%s *c,', self.c_reply_fds_name, spacing1)
> > > > + _h('%s%s *reply);', spacing3, self.c_reply_type)
> > > > + _c('%s%s *reply)', spacing3, self.c_reply_type)
> > > > _c('{')
> > > >
> > > > _c(' return xcb_get_reply_fds(c, reply, sizeof(%s) + 4 * reply->length);', \
> > > > self.c_reply_type) @@ -2221,7 +2221,7 @@ def _c_cookie(self, name):
> > > > _h(' * @brief %s', self.c_cookie_type)
> > > > _h(' **/')
> > > > _h('typedef struct %s {', self.c_cookie_type)
> > > > - _h(' unsigned int sequence; /**< */')
> > > > + _h(' unsigned int sequence;')
> > > > _h('} %s;', self.c_cookie_type)
> > > >
> > > > def _man_request(self, name, cookie_type, void, aux):
> > > > @@ -2304,7 +2304,7 @@ def _man_request(self, name, cookie_type, void, aux):
> > > > else:
> > > > spacing = ' ' * (maxtypelen - (len(field.c_field_type) + 1))
> > > > f.write('ELSE %s = %s\n' % (field.c_field_type, field.c_field_name))
> > > > - #_h('%s %s%s *%s%s; /**< */', space, field.c_field_type, \
> > > > spacing, field.c_field_name, field.c_subscript) + #_h('%s \
> > > > %s%s *%s%s;', space, field.c_field_type, spacing, field.c_field_name, \
> > > > field.c_subscript)
> > > > if not self.is_switch:
> > > > for field in struct_fields:
> _______________________________________________
> Xcb mailing list
> Xcb@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/xcb
_______________________________________________
Xcb mailing list
Xcb@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/xcb
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic