[Xcb] [PATCH libxcb 1/6] generator: fix absname for fields with only accessor function
Ran Benita
ran234 at gmail.com
Tue Oct 14 06:24:38 PDT 2014
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