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

[Christian Linhart]

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:
[...]
>>> 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). 
OK. I agree with you that this should not happen.
We should address this later as it is unrelated to this patchset.

For that: Do you have a testcase where such garbage is generated, 
together with info which garbage is generated, and why this is garbage?

[...]
>>>>      """
>>>>      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:
>>>
[...]
> 
> 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).
Interesting.
Do you remember where sep is empty on top of my patch set?
If you don't remember it, no problem, I'll test this myself.
Maybe this hints to a bug.

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

I have to analyze the code that does his kind of decision elsewhere.
Depending on that code, a state variable may be better than a function or vice versa.

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

Thank you.

If you have questions please tell me and I'll be glad to answer them.

If I have time, I'll look into it and check for possibilities of simplification.

[...]
>>> 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.
I have put this on my simplification-TODO list and will rename it once everything is merged in upstream.

> 
>>> 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 :) 
Yes, I'll do that.
Once we have the functionality in there, 
I am also very interested in a cleanup,
so that maintenance or adding new functionality
in the future will be easier 
and so that we can be more sure about the
reliability and correctness of the whole thing.

(My X11 related business is about missing critical code,
so I have every interest in reliability of the code.
Also, I therefore operate quite differently from
a typical commercial software company.
I have a zero-known-bug release policy for example, ...)

> 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"...
Yep, there is some technical debt in this code,
but I have seen far worse cases of technical debt.
So I think it is still in a state that can be repaired.

Or to put it in a bit more positive way, 
previously some of this code was written
with a pioneer attitude to get things working quickly
by using a kind of rapid-prototyping-style.

This may have been useful for the project to gain some traction,
but left some debt which needs cleanup.

Actually most commercial code is developed that way, but without the cleanup state,
so left in a messy state until the software has to be rewritten from scratch
or the company goes belly-up due to exponentially growing maintenance cost.
The typical manager speak is: "We agree that cleanup is useful. But we
have 23428 more urgent things to do now. We'll do that later." 
Of course "Later" never happens.

I am glad we don't have this kind of pressure and "guidance" 
in a free software project like this.

Chris