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.
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".
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.
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.
----- Original Message -----
From: "Jakub Hrozek" jhrozek@redhat.com To: sssd-devel@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.
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.
On Wed, Jan 21, 2015 at 11:35:05AM -0500, Pavel Brezina wrote:
----- Original Message -----
From: "Jakub Hrozek" jhrozek@redhat.com To: sssd-devel@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.
----- Original Message -----
From: "Jakub Hrozek" jhrozek@redhat.com To: sssd-devel@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@redhat.com To: sssd-devel@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.
On Wed, Jan 21, 2015 at 04:31:24PM -0500, Pavel Brezina wrote:
----- Original Message -----
From: "Jakub Hrozek" jhrozek@redhat.com To: sssd-devel@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@redhat.com To: sssd-devel@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..
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@redhat.com To: sssd-devel@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@redhat.com To: sssd-devel@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.
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@redhat.com To: sssd-devel@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@redhat.com To: sssd-devel@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...
On Thu, Jan 22, 2015 at 01:45:59PM +0100, Pavel Březina wrote:
Attached is a patch without macros...
Awesome, this is really good. One last question before I ack: Do you think the sbus_get_invoker_fn() typedef could use a new typedef that expands to: void (*handler_fn)(struct sbus_request *, void *, void *); instead of the void* function pointer? I suppose that would make the code more type-safe..
I really like to see all the cruft going away :-)
On Mon, Feb 16, 2015 at 04:27:17PM +0100, Jakub Hrozek wrote:
On Thu, Jan 22, 2015 at 01:45:59PM +0100, Pavel Březina wrote:
Attached is a patch without macros...
Awesome, this is really good. One last question before I ack: Do you think the sbus_get_invoker_fn() typedef could use a new typedef that expands to: void (*handler_fn)(struct sbus_request *, void *, void *); instead of the void* function pointer? I suppose that would make the code more type-safe..
I really like to see all the cruft going away :-)
CI also passed: http://sssd-ci.duckdns.org/logs/job/7/43/summary.html
On 02/16/2015 04:27 PM, Jakub Hrozek wrote:
On Thu, Jan 22, 2015 at 01:45:59PM +0100, Pavel Březina wrote:
Attached is a patch without macros...
Awesome, this is really good. One last question before I ack: Do you think the sbus_get_invoker_fn() typedef could use a new typedef that expands to: void (*handler_fn)(struct sbus_request *, void *, void *); instead of the void* function pointer? I suppose that would make the code more type-safe..
I really like to see all the cruft going away :-)
No, its not doable unfortunately.
Basic types, such as strings, integers, ... uses the following parameters: void (*handler_fn)(struct sbus_request *, void *, void *)
but array types uses: void (*handler_fn)(struct sbus_request *, void *, void *, int *)
So we can't really specify precise type unless we want to have also different invoker types which would result in more code branching.
On Tue, Feb 17, 2015 at 03:03:48PM +0100, Pavel Březina wrote:
On 02/16/2015 04:27 PM, Jakub Hrozek wrote:
On Thu, Jan 22, 2015 at 01:45:59PM +0100, Pavel Březina wrote:
Attached is a patch without macros...
Awesome, this is really good. One last question before I ack: Do you think the sbus_get_invoker_fn() typedef could use a new typedef that expands to: void (*handler_fn)(struct sbus_request *, void *, void *); instead of the void* function pointer? I suppose that would make the code more type-safe..
I really like to see all the cruft going away :-)
No, its not doable unfortunately.
Basic types, such as strings, integers, ... uses the following parameters: void (*handler_fn)(struct sbus_request *, void *, void *)
but array types uses: void (*handler_fn)(struct sbus_request *, void *, void *, int *)
So we can't really specify precise type unless we want to have also different invoker types which would result in more code branching.
Thank you for the explanation.
In that case, ACK to the patches. Very good work removing the legacy code!
On Tue, Feb 17, 2015 at 03:08:18PM +0100, Jakub Hrozek wrote:
On Tue, Feb 17, 2015 at 03:03:48PM +0100, Pavel Březina wrote:
On 02/16/2015 04:27 PM, Jakub Hrozek wrote:
On Thu, Jan 22, 2015 at 01:45:59PM +0100, Pavel Březina wrote:
Attached is a patch without macros...
Awesome, this is really good. One last question before I ack: Do you think the sbus_get_invoker_fn() typedef could use a new typedef that expands to: void (*handler_fn)(struct sbus_request *, void *, void *); instead of the void* function pointer? I suppose that would make the code more type-safe..
I really like to see all the cruft going away :-)
No, its not doable unfortunately.
Basic types, such as strings, integers, ... uses the following parameters: void (*handler_fn)(struct sbus_request *, void *, void *)
but array types uses: void (*handler_fn)(struct sbus_request *, void *, void *, int *)
So we can't really specify precise type unless we want to have also different invoker types which would result in more code branching.
Thank you for the explanation.
In that case, ACK to the patches. Very good work removing the legacy code!
* master: 16cf65323a41b3de4a98f309cdf900f3b0d9ec25 df4e1db5d41c903ae57fd880acc76a0ad84aa7b2
On Thu, Jan 22, 2015 at 12:19:52PM +0100, 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@redhat.com To: sssd-devel@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@redhat.com To: sssd-devel@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.
Do we need to modify the codegen for the dbus responder work, in order to support any additional data types? If not and this is just convenience change, then it's a nack..
Even if we do need changes, wouldn't it be better to fix/modify codegen?
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
This is not the issue /at all/. I'm not too fond of the codegen and I especially *hate* the add_XXX_as_YYY() functions I wrote. Removing the add_XXX_as_YYY() functions is the thing I actually /like/ about these patches.
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.
My problem is debuggability. If something goes bad in macro generated code, it's completely impossible to walk the code in gdb or tell what went wrong from a backtrace. Normally the only way to debug macro-generated code is to preprocess the C file, compile the preprocessed result and debug that...both me and Lukas still have scars from doing exactly that in nss-pam-ldapd..
If (and only if) you think changing codegen to support your next development is impossible, then we could decorate the macros with DEBUG statements so that every branch is covered and change them so that they assign dbus return code to an output variable (no changing code flow in macros, that's a nack, sorry) and then convert the output dbus return code into something specific in another function in the caller.
Right now, there is an opaque blob of code that just returns EIO when something goes wrong..
Plus, we need tests for all the types, but I think we already have those..
On 02/11/2015 02:13 PM, Jakub Hrozek wrote:
On Thu, Jan 22, 2015 at 12:19:52PM +0100, 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@redhat.com To: sssd-devel@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@redhat.com > To: sssd-devel@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.
Do we need to modify the codegen for the dbus responder work, in order to support any additional data types?
Yes.
If not and this is just
convenience change, then it's a nack..
Even if we do need changes, wouldn't it be better to fix/modify codegen?
I strongly disagree with generating something that just doesn't have to be generated. C code is always better than C code in Python.
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
This is not the issue /at all/. I'm not too fond of the codegen and I especially *hate* the add_XXX_as_YYY() functions I wrote. Removing the add_XXX_as_YYY() functions is the thing I actually /like/ about these patches.
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.
My problem is debuggability. If something goes bad in macro generated code, it's completely impossible to walk the code in gdb or tell what went wrong from a backtrace. Normally the only way to debug macro-generated code is to preprocess the C file, compile the preprocessed result and debug that...both me and Lukas still have scars from doing exactly that in nss-pam-ldapd..
There are no macros in the latest patch...
If (and only if) you think changing codegen to support your next development is impossible, then we could decorate the macros with DEBUG statements so that every branch is covered and change them so that they assign dbus return code to an output variable (no changing code flow in macros, that's a nack, sorry) and then convert the output dbus return code into something specific in another function in the caller.
Right now, there is an opaque blob of code that just returns EIO when something goes wrong..
Plus, we need tests for all the types, but I think we already have those..
All tests are there, unchanged and working.
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Pavel Březina