On 01/22/2015 12:19 PM, Pavel Březina wrote:
On 01/21/2015 10:42 PM, Jakub Hrozek wrote:
> On Wed, Jan 21, 2015 at 04:31:24PM -0500, Pavel Brezina wrote:
>>
>>
>> ----- Original Message -----
>>> From: "Jakub Hrozek" <jhrozek(a)redhat.com>
>>> To: sssd-devel(a)lists.fedorahosted.org
>>> Sent: Wednesday, January 21, 2015 10:00:07 PM
>>> Subject: Re: [SSSD] [PATCH] sbus: use hard coded getters instead of
>>> generated
>>>
>>> On Wed, Jan 21, 2015 at 11:35:05AM -0500, Pavel Brezina wrote:
>>>>
>>>>
>>>> ----- Original Message -----
>>>>> From: "Jakub Hrozek" <jhrozek(a)redhat.com>
>>>>> To: sssd-devel(a)lists.fedorahosted.org
>>>>> Sent: Wednesday, January 21, 2015 3:55:24 PM
>>>>> Subject: Re: [SSSD] [PATCH] sbus: use hard coded getters instead of
>>>>> generated
>>>>>
>>>>> On Tue, Jan 20, 2015 at 09:54:56PM +0100, Pavel Březina wrote:
>>>>>> On 01/05/2015 02:04 PM, Pavel Březina wrote:
>>>>>>> Hi,
>>>>>>> I wrote most of this patch during the Christmas break.
>>>>>>>
>>>>>>> It removes the codegen code for generating property getters
>>>>>>> (invokers
>>>>>>> to
>>>>>>> be precise) and create hard coded ones. It also add a simple
and
>>>>>>> general
>>>>>>> purpose GetAll invoker. The reason and justification is in
the
>>>>>>> commit
>>>>>>> message.
>>>>>>>
>>>>>>> Keeping method generated invokers makes sense, since each
method
>>>>>>> can
>>>>>>> have different number of arguments with different type.
>>>>>>>
>>>>>>> I hope you like it.
>>>>>>
>>>>>> Rebased on top of latest respin of "[PATCHES] sbus: support
multiple
>>>>>> interfaces on single object path".
>>>>>>
>>>>>
>>>>> I agree with the general intention, especially considering the
>>>>> removal
>>>>> of the reply as functions in the second patch, but I really dislike
>>>>> removing 800 lines of tests without a proper substitute.
>>>>
>>>> There is not test remove, it is just generated code that changed.
>>>
>>> Aah, sorry, I should have read the patches better.
>>>
>>>>
>>>>> Also, the 30+ lines long macros are impossible to debug, please
>>>>> convert
>>>>> them to functions. Given that callers of the macros usually just
call
>>>>> the single line, you can just return the new function.
>>>>
>>>> The reason it is a macro is to change the type of variable and
>>>> handler. It
>>>> is not possible to do it with a function.
>>>
>>> Could we get away with a void pointer and pass the dbus_type as void* ?
>>>
>>> Then I don't see the point in the patches except the removal of
>>> the reply-as functions. You pretty much just substituted one generator
>>> (the python script) for another (the macro).
>>>
>>> The upside is being less code, sure.
>>>
>>> The downside being the macros are much harder to debug..I really don't
>>> like huge amounts of code generated by macros. To see the horrors this
>>> can lead to, open the libc code, nsswitch in particular. If you change
>>> the python generator, it's much easier to debug the resulting C code.
>>> OTOH if you change the macro, you need to invoke the preprocessor to
>>> see
>>> what's going on and that's...nasty.
>>>
>>> Maybe I'm negatively influenced by reading the nsswitch code lately,
>>> but honestly, I'd much rather keep the python codegen.
>>
>> The point is that it would be hard and error proof to add new,
>> complex types to the generator (such as a{sas} for extra attributes).
>> I'll try to go with the void pointer.
>
> Can you explain better? I don't want to shoot down some proposal just
> because I'm missing part of some research you did..
First of all I don't like the idea of generating code that just doesn't
have to be generated. All the getters needs to be present anyway, since
they are contained within a unit test. Therefore generating the code
doesn't bring any advantage here, it is just a duplicated code. It make
sense for methods because each method may have different arguments so it
is not possible to write all of them down but number of property types
is small.
As I see it the main disadvantage is that the code generator is already
hard to read. It uses the same template for all types and then makes
modifications for array, strings, object paths, bool, ... And those are
just simple types. Besides you don't have access to C code highlighter,
outline and other IDE features in Python. Modifying the codegen to take
more complex types would be quite error prone and I bet you will write
it in pure C first and then put it into the codegen anyway.
The basic types are all in there, the complex types will be written as
needed. But you would have to write them in codegen as well. In C, it
will be just easier.
At first, I just wanted to get rid of get all generator, since we have
all information necessary available in sbus meta structures. I'm sorry,
I know its your code and I acked it some time ago, but neither of us
actually liked it. I saw a way how to do it better now. At least in my
opinion. It needed modification of getters though so I went a step
forward an got rid of this codegen part as well.
Attached is a patch without macros...