[Xcb] [PATCH libxcb 11/18] c_client.py: don't generate useless empty /** < */ comments
Christian Linhart
chris at DemoRecorder.com
Wed Oct 29 00:33:01 PDT 2014
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 at 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:
>>
>
More information about the Xcb
mailing list