See patch comments for details. All three are not massive. 1) Comparison and sorting collections 2) Taking part of the code from long module and putting it into a new module for readability 3) Adding new functionality to the module created in previous patch
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 09/15/2009 09:20 PM, Dmitri Pal wrote:
- Comparison and sorting collections
Did you maybe forget to git-add a file? Makefile.am references collection_cmp.c but that file is in none of the patches.
Jakub
Jakub Hrozek wrote:
On 09/15/2009 09:20 PM, Dmitri Pal wrote:
- Comparison and sorting collections
Did you maybe forget to git-add a file? Makefile.am references collection_cmp.c but that file is in none of the patches.
Jakub
Thank you, yes I will resend.
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
Dmitri Pal wrote:
See patch comments for details. All three are not massive.
- Comparison and sorting collections
- Taking part of the code from long module and putting it into a new
module for readability 3) Adding new functionality to the module created in previous patch
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
Resending the first patch which (as Jakub noticed) missed the file. Sorry for inconvenience.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 09/17/2009 06:08 AM, Dmitri Pal wrote:
Dmitri Pal wrote:
See patch comments for details. All three are not massive.
- Comparison and sorting collections
I don't understand why are you testing for "first->property[second->property_len] != '\0'" when testing for COL_CMPIN_PROP_DOT? Does that mean that comparing the exactly same properties with _BEG and COL_CMPIN_PROP_DOT would compare OK (In other words, I care about dot only with strings that are not equal)?
In comparing COL_TYPE_BINARY, after "if (first->length == second->length)", I think you are missing "else result = NONZERO;"
For other types, I'm wondering if the code could be somehow refactored (with a macro perhaps), it's essentially 6 blocks of the very same code except for the cast.
The swapped != is certainly a bug: if ((res =! 0) && (out_flags == 0)) { if ((res =! 0) && (out_flags == 0)) {
In general, is it possible to use some other sorting algorithm than the one used? It appears to be a variant of bubble sort which is not very effective. Maybe qsort from the standard C library could be used, but I'm not sure how to deal with the flags..maybe a wrapper could be used (as qsort only works based on return value)
- Taking part of the code from long module and putting it into a new
module for readability
Ack, just moves code around
- Adding new functionality to the module created in previous patch
Looks good to me.
On Thu, 2009-09-17 at 19:21 +0200, Jakub Hrozek wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 09/17/2009 06:08 AM, Dmitri Pal wrote:
Dmitri Pal wrote:
See patch comments for details. All three are not massive.
- Comparison and sorting collections
The swapped != is certainly a bug: if ((res =! 0) && (out_flags == 0)) { if ((res =! 0) && (out_flags == 0)) {
I would advise against unneeded parentheses like in this case. If you wouldn't use them the compiler would warn you.
Martin
Martin Nagy wrote:
On Thu, 2009-09-17 at 19:21 +0200, Jakub Hrozek wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 09/17/2009 06:08 AM, Dmitri Pal wrote:
Dmitri Pal wrote:
See patch comments for details. All three are not massive.
- Comparison and sorting collections
The swapped != is certainly a bug: if ((res =! 0) && (out_flags == 0)) { if ((res =! 0) && (out_flags == 0)) {
I would advise against unneeded parentheses like in this case. If you wouldn't use them the compiler would warn you.
Martin
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
Are you sure they are unneeded? I will give it a try some time. It is just a habit of enclosing every logical statement in a separate pair of parentheses for me it helps to mentally evaluate the statement as I write it. I already do not remember whether it is just me and my habit or once was cause by some compiler reasons.
Jakub Hrozek wrote:
On 09/17/2009 06:08 AM, Dmitri Pal wrote:
Dmitri Pal wrote:
See patch comments for details. All three are not massive.
- Comparison and sorting collections
I don't understand why are you testing for "first->property[second->property_len] != '\0'" when testing for COL_CMPIN_PROP_DOT? Does that mean that comparing the exactly same properties with _BEG and COL_CMPIN_PROP_DOT would compare OK (In other words, I care about dot only with strings that are not equal)?
Ok think about the case when you have a flattened collection. Imagine you have elements: bar barcode bar.drink1 bar.drink2
With _BEG but not dot it will match all of them. With _BEG and _DOT it will match "bar", "bar.drink1" and "bar.drink1" And this is what I am trying to accomplish at least this is how I envision it to be used. That being said there is no use case so far and this functionality is not used. When we get into a real case of _BEG, _MID and _END the functionality may be adjusted.
In comparing COL_TYPE_BINARY, after "if (first->length == second->length)", I think you are missing "else result = NONZERO;"
Agree. Will fix.
For other types, I'm wondering if the code could be somehow refactored (with a macro perhaps), it's essentially 6 blocks of the very same code except for the cast.
I will think about it. Macro is a good idea...
The swapped != is certainly a bug: if ((res =! 0) && (out_flags == 0)) { if ((res =! 0) && (out_flags == 0)) {
Auch... Yes.
In general, is it possible to use some other sorting algorithm than the one used? It appears to be a variant of bubble sort which is not very effective. Maybe qsort from the standard C library could be used, but I'm not sure how to deal with the flags..maybe a wrapper could be used (as qsort only works based on return value)
Sure. I needed something quick and dirty that I can put together quickly to test the comparisons. I will open a ticket but so far it is not critical since sorting is used only for testing comparisons.
- Taking part of the code from long module and putting it into a new
module for readability
Ack, just moves code around
- Adding new functionality to the module created in previous patch
Looks good to me.
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
Resending COLLECTION related patches with issues addressed except enhancing sorting. For that I opened a ticket. There is one more patch with new functions and couple of enhancements added in comparison to the previous one.
Dmitri Pal wrote:
Jakub Hrozek wrote:
On 09/17/2009 06:08 AM, Dmitri Pal wrote:
Dmitri Pal wrote:
See patch comments for details. All three are not massive.
- Comparison and sorting collections
I don't understand why are you testing for "first->property[second->property_len] != '\0'" when testing for COL_CMPIN_PROP_DOT? Does that mean that comparing the exactly same properties with _BEG and COL_CMPIN_PROP_DOT would compare OK (In other words, I care about dot only with strings that are not equal)?
Ok think about the case when you have a flattened collection. Imagine you have elements: bar barcode bar.drink1 bar.drink2
With _BEG but not dot it will match all of them. With _BEG and _DOT it will match "bar", "bar.drink1" and "bar.drink1" And this is what I am trying to accomplish at least this is how I envision it to be used. That being said there is no use case so far and this functionality is not used. When we get into a real case of _BEG, _MID and _END the functionality may be adjusted.
In comparing COL_TYPE_BINARY, after "if (first->length == second->length)", I think you are missing "else result = NONZERO;"
Agree. Will fix.
For other types, I'm wondering if the code could be somehow refactored (with a macro perhaps), it's essentially 6 blocks of the very same code except for the cast.
I will think about it. Macro is a good idea...
The swapped != is certainly a bug: if ((res =! 0) && (out_flags == 0)) { if ((res =! 0) && (out_flags == 0)) {
Auch... Yes.
In general, is it possible to use some other sorting algorithm than the one used? It appears to be a variant of bubble sort which is not very effective. Maybe qsort from the standard C library could be used, but I'm not sure how to deal with the flags..maybe a wrapper could be used (as qsort only works based on return value)
Sure. I needed something quick and dirty that I can put together quickly to test the comparisons. I will open a ticket but so far it is not critical since sorting is used only for testing comparisons.
- Taking part of the code from long module and putting it into a new
module for readability
Ack, just moves code around
- Adding new functionality to the module created in previous patch
Looks good to me.
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 09/28/2009 04:56 AM, Dmitri Pal wrote:
Resending COLLECTION related patches with issues addressed except enhancing sorting. For that I opened a ticket. There is one more patch with new functions and couple of enhancements added in comparison to the previous one.
Patch 0001 - just one thing, please wrap the multi-line macro TYPED_MATCH() in a do..while(0)
Patches 0002 and 0003 are the same as before - ack Patch 0004 - ack
Jakub
Jakub Hrozek wrote:
On 09/28/2009 04:56 AM, Dmitri Pal wrote:
Resending COLLECTION related patches with issues addressed except enhancing sorting. For that I opened a ticket. There is one more patch with new functions and couple of enhancements added in comparison to the previous one.
Patch 0001 - just one thing, please wrap the multi-line macro TYPED_MATCH() in a do..while(0)
Done
Resending.
Patches 0002 and 0003 are the same as before - ack Patch 0004 - ack
Jakub
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 10/02/2009 06:47 PM, Dmitri Pal wrote:
Patch 0001 - just one thing, please wrap the multi-line macro TYPED_MATCH() in a do..while(0)
Done
Resending.
Ack
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 10/05/2009 06:27 AM, Jakub Hrozek wrote:
On 10/02/2009 06:47 PM, Dmitri Pal wrote:
Patch 0001 - just one thing, please wrap the multi-line macro TYPED_MATCH() in a do..while(0)
Done
Resending.
Ack
Pushed to master. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
- -- Stephen Gallagher RHCE 804006346421761
Looking to carve out IT costs? www.redhat.com/carveoutcosts/
sssd-devel@lists.fedorahosted.org