[SSSD] [PATCH] sbus: use hard coded getters instead of generated

Pavel Březina pbrezina at redhat.com
Thu Jan 22 12:45:59 UTC 2015


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 at redhat.com>
>>>> To: sssd-devel at 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 at redhat.com>
>>>>>> To: sssd-devel at 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...


-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-sbus-use-hard-coded-getters-instead-of-generated.patch
Type: text/x-patch
Size: 111111 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20150122/0791742d/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-sbus-remove-unused-reply-as-functions.patch
Type: text/x-patch
Size: 13505 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20150122/0791742d/attachment-0001.bin>


More information about the sssd-devel mailing list