[Xcb] Renamed and simplified function _c_helper_absolute_name / Temp repo updated
Christian Linhart
chris at DemoRecorder.com
Thu Oct 30 08:21:25 PDT 2014
Hi Ran,
Today I had finally time to analyze, simplify and rename
the function "_c_helper_absolute_name" aka "_c_helper_fieldaccess_expr".
I hope this all makes the code better and reviewing easier.
See the patches that I have just posted for this patchset
(and one patch in a subsequent patchset to adapt for the changes.)
All these simplifications do not alter the output of the generator.
(That's a nice thing about xcb: You can test refactorings in the
generator easily by comparing the generated code with "diff".)
I have also updated the temp-repo accordingly.
http://infra-srv1.demorecorder.com/git/free-sw/xcb/libxcb
http://infra-srv1.demorecorder.com/git/free-sw/xcb/proto
Here's a summary of what I have done:
* I have experimented with the long list of conditions
of one "if" in this function ( as we have discussed in this thread ).
After some debugging I have found out that most conditions
are actually a workaround for a bug elsewhere:
The function "_c_type_setup" has recursed into a field-object
*before* setting some essential member-variables of this field-object.
The function "_c_helper_fieldaccess_expr" is called indirectly there.
Several sub-conditions of the long if-condition had the purpose
to filter away those fields which have an unfinished parent
or which are themselves unfinished.
If have fixed the recursion in "_c_type_setup" so that the recursion
is done *after* the member-variables of the field-object have been set.
( see patch "libxcb 1/6 V3" )
So, Ran, you had the right gut feeling that there's something
wrong with this long list of conditions. Thank you!
Maybe, fixing this recursion-bug will also reduce the
internal garbage generated by this function?
( I cannot tell because I don't know what kind of
garbage it has generated. )
* then, I have extracted the remaining conditions into functions
with meaningful names. ( see patch "libxcb 1/6 V3" )
( I have also replaced these conditions with function calls
in function "_c_accessors". There are more places where
these conditions are used. I'll replace them with the function
calls, too, in an extra cleanup-patchset which I'll post later.)
* after these simplifications I saw some more ways to simplify
the control-flow of this function.
( see patch "libxcb 1/6 V3" )
* I have renamed function "_c_helper_absolute_name"
to "_c_helper_fieldaccess_expr" as we have discussed.
and improved the description of this function.
( patches "libxcb 7/9" and "libxcb 8/9" )
* I have identified and fixed the place which caused "sep" to be empty.
( patch "libxcb 5/6 V2" of this patchset
and patch "libxcb 4/4 V3" of patchset "PopcountList" )
Therefore I could remove the handling for "sep" being empty.
( patch "libxcb 9/9" )
I hope this helps with code-quality and reviewing.
Chris
On 10/14/14 15:24, Ran Benita wrote:
> On Mon, Oct 13, 2014 at 03:33:23PM +0200, Christian Linhart wrote:
>> On 10/12/14 20:07, Ran Benita wrote:
>>> On Wed, Sep 03, 2014 at 01:22:36PM +0200, Christian Linhart wrote:
>>>> Fix _c_helper_absolute_name for fields which cannot be accessed
>>>> as a struct/union member but which can be accessed by an
>>>> accessor function.
>>>>
>>>> The fix calls the accessor function in these cases.
>>>>
>>>> Example:
>>>> <struct name="AbsnameTest">
>>>> <field type="CARD32" name="len" />
>>>> <list type="CARD8" name="mylist1">
>>>> <fieldref>len</fieldref>
>>>> </list>
>>>> <list type="CARD8" name="mylist2">
>>>> <sumof ref="mylist1"/>
>>>> </list>
>>>> </struct>
>>>>
>>>> The sumof-expression ( <sumof ref="mylist1"/> ) refers to mylist1
>>>> which is only acessible by an accessor function.
>>>>
>>>> Previously, sumof was only used inside bitcases,
>>>> where such lists are accessible by members of the
>>>> deserialized parent struct.
>>>> (there is a difference between deserialization of switches
>>>> and structs.)
>>>> ---
>>>> src/c_client.py | 28 +++++++++++++++++++++++++++-
>>>> 1 file changed, 27 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/src/c_client.py b/src/c_client.py
>>>> index 9216c7f..488f0e6 100644
>>>> --- a/src/c_client.py
>>>> +++ b/src/c_client.py
>>>> @@ -430,24 +430,50 @@ def _c_type_setup(self, name, postfix):
>>>> def _c_helper_absolute_name(prefix, field=None):
>>>
>>> This function is strange; most of the time it generates complete garbage
>>> (e.g. `foo->...->`) which is subsequently ignored. So it is called for
>>> no reason many times.
>> I do not understand the problem here,
>
> Basically, "garbage in, garbage out". The problem is the "garbage in"
> part - it shouldn't happen in well-structured code... (of course this
> is well before this patchset). So that makes it hard to figure out what
> it *does*. This patch:
> http://lists.freedesktop.org/archives/xcb/2014-October/009939.html
> removes some garbage calls, but there are still a lot left.
>
>> The output *is* used sometimes and that counts for the output.
>>
>> In most cases a symbol-resolution table is filled with all fields
>> that are accessible starting from a given scope/type.
>> For each field, the C-expression which accesses the field, is computed here.
>> Of course, only a subset of that symbol-resolution table is used later.
>> In that sense, most of the output is subsequently ignored.
>
> Yes, that's ok. My problem is that some entires are nonsense!
>
>> ( for example the function "_c_helper_field_mapping" builds such a table )
>>
>>>
>>>> """
>>>> turn prefix, which is a list of tuples (name, separator, Type obj) into a string
>>>> representing a valid name in C (based on the context)
>>>> if field is not None, append the field name as well
>>>> """
>>>> prefix_str = ''
>>>> + last_sep =''
>>>> for name, sep, obj in prefix:
>>>> + if '' != last_sep:
>>>> + prefix_str += last_sep
>>>> prefix_str += name
>>>> if '' == sep:
>>>
>>> (This can never happen. I have a patch to remove this).
>> By quick analysis I can confirm that because the blank
>> seps are already filled in _c_helper_resolve_field_names.
>>
>> How did you come to the conclusion that sep can never be the empty string here?
>>
>> Since this is old code not affected by my patch
>> and since you didn't post your patch yet as far as I can tell,
>> I suggest that you post your patch after this patchset has been merged into upstream.
>> ( to avoid unnecessary merge-work )
>>
>> Of course I could include that change in V2 of that patch but that way
>> we would have two unrelated changes in one patch which is not good.
>
> I looked over the callers, and saw that `sep` is never empty. I had a
> patch which added `assert sep != ''`. This worked on master, but failed
> on top of your patch set. It is probably more "GIGO" but I dropped it
> (but forgot to amend this comment).
>
>>>
>>>> sep = '->'
>>>> if ((obj.is_case_or_bitcase and obj.has_name) or # named bitcase
>>>> (obj.is_switch and len(obj.parents)>1)):
>>>> sep = '.'
>>>> - prefix_str += sep
>>>> + last_sep = sep
>>>> +
>>>> + prefix_str_without_lastsep = prefix_str
>>>> + prefix_str += last_sep
>>>> +
>>>> if field is not None:
>>>> prefix_str += _cpp(field.field_name)
>>>> +
>>>> +
>>>> + if (
>>>
>>> This is a formidable list of conditions. Are all of them needed?
>> I think that yes they are all needed, but there may be a better alternative ( see below ).
>>
>>> Can
>>> this be extracted to a function `field_needs_accessor` or something like
>>> that?
>>
>> I have thought about that and came to the conclusion that there probably is an even better alternative
>> than to encapsulate those conditions in a function:
>>
>> Probably it is better to set a flag at the place where it has already been decided that this field
>> does not appear in a C-struct. ( or use the reverse semantic )
>> Then just use that flag here.
>> That would have the advantage that these conditions need not be updated when something is changed somewhere else.
>
> I wouldn't advocate a state variable over a pure function. But if it is
> constant that's the same.
>
>> Would it be OK to use this patch as it is now
>> and that I post a simplification fix later,
>> after this is merged to upstream?
>>
>> Or shall I submit V2 of this patch, with changes as I have suggested above?
>> ( This will probably take some time ).
>
> I'll try to understand the given code.
>
>>>
>>>> + field != None
>>>
>>> With `None` it is customary to write `field is None`, `field is not
>>> None`.
>> Yes, I am sorry for that.
>> I have noticed that in the meantime.
>>
>>>
>>>> + and hasattr(field, 'c_accessor_name')
>>>> + and field.parent != None
>>>> + and field.parent.is_container
>>>> + and not field.parent.is_switch
>>>> + and not field.parent.is_case_or_bitcase
>>>> + and ( #the following conditions are taken from _c_accessors()
>>>> + ( field.type.is_list and not field.type.fixed_size() )
>>>> + or
>>>> + ( field.prev_varsized_field is not None
>>>> + or not field.type.fixed_size()
>>>
>>> This says `or` but the parens make it look like it should be `and`?
>> I have copied that condition from function _c_accessors and it is used like that
>> there. The outer "or" combines the condition of an "if" and the condition of an "elif".
>> Of course, boolean algebra would allow to remove the parentheses around the inner or.
>>
>> It is important that this is consistent with "_c_accessors".
>
> OK, this just looked "suspicous", like the for loop described in this
> article: http://lwn.net/Articles/453685/
>
>>
>>>
>>>> + )
>>>> + )
>>>> + ):
>>>> + prefix_str = field.c_accessor_name + "(" + prefix_str_without_lastsep + ")";
>>>
>>> I'm not entirely convinced that this should be in this function - or the
>>> function should be renamed.
>> Renaming is a good idea for that function anyways.
>> Even the existing version of that function does not compute an "absolute name" but computes a C-expression
>> to access a specific field in the context defined by the "prefix" parameter.
>>
>> I suggest the following name: "_c_helper_field_access_expr"
>> ( or something similar)
>
> If this name is accurate, that would be a big improvement.
>
>>> Is the above the correct thing to do in all
>>> of the callsites?
>> Good question.
>>
>> I am pretty sure that the answer is yes, because of the following:
>>
>> * The previous version of that function returned a C-expression that can also be used as an lvalue, i.e., C can assign something to it.
>>
>> * Due to my patches, the returned C-expression may be an rvalue in those cases where the old function returned something that cannot compile.
>> ( i.e. these are the cases where the field does not occur in a C-struct)
>>
>> * So, the behavior only changes for those cases where the existing version of that function returns something that is not compilable.
>> So, it doesn't break anything.
>>
>> * If, in those cases, the output of that function were somewhere inserted in the role of an lvalue,
>> the C-compiler would complain.
>> But the Compiler does not complain, which means that the resulting C-expression is only used as an rvalue.
>> ( at least for all of our current xml-files. )
>>
>> I hope that this explains it better.
>>
>> If you have more questions, please tell me and I'll be glad to answer them.
>>
>>>
>>> Sorry for having more questions than answers; it is not for lack of
>>> trying..
>> No problem.
>> I wrote the change, so I have find the answers... :-)
>
> I will take another look. If someday you can simplify this, it would be
> nice :) I hate to say this, but you are paying off some technical debt
> left by a previous programmer whose code may be found with "git blame"...
>
> Ran
>
>> Chris
>>
>>>
>>> Ran
>>>
>>>> +
>>>> return prefix_str
>>>> # _c_absolute_name
>>>>
>>>> def _c_helper_field_mapping(complex_type, prefix, flat=False):
>>>> """
>>>> generate absolute names, based on prefix, for all fields starting from complex_type
>>>> if flat == True, nested complex types are not taken into account
>>>> --
>>>> 2.0.1
>>>>
>>>> _______________________________________________
>>>> Xcb mailing list
>>>> Xcb at lists.freedesktop.org
>>>> http://lists.freedesktop.org/mailman/listinfo/xcb
>>>
>>
>
More information about the Xcb
mailing list