[Xcb] [PATCH libxcb 1/6] generator: fix absname for fields with only accessor function

[Christian Linhart]

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,
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.

( 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. 

> 
>>              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.

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 ).

> 
>> +        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".

> 
>> +                )
>> +        )
>> +    ):
>> +        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)

> 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... :-)

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
>