Stephen Gallagher wrote:
On 01/05/2011 03:40 PM, Dmitri Pal wrote:
>>>>> 0003--BUILD-Allow-trace-per-component.patch <- This patch allows
>> tracing
>>>>> per component
>>>> Nack. Requiring a file doesn't make sense. Just add
--enable-trace-ini,
>>>> --enable-trace-collection etc. flags.
>>> This is exactly what I did not like to do and tried to avoid. It
is more
>>> code on the configure script, more checks and less convenient to
turn on
>>> and off becuase you can run the same configure command with one switch
>>> enabled from history and do "touch trace" & "rm
trace" as needed
in the
>>> directory you need.
>>> Saves a lot of typing for me :-)
>>> If you do not like the patch I will have to keep it as private
patch as
>>> it is more convenient for me during development than the flags.
>>
>> I still vote nack, as I don't like the approach.
> So this is the dead lock becuase I like the approach and do not like
> yours. How we are going to resolve it?
Ultimately, it's your project, not mine. You have commit privilege and
full discretion on when to use it. You asked for my review comments. You
don't have to listen to them :)
Nack. I think we need to come to some terms and find consensus. I will
definitely not push it over your head using my privilege.
I agree that what you suggest is logical and consistent with how things
are done. The problem is that it does not help me much.
I will keep patch in my tree for now.
>>
>>>>> 0005--INI-New-merge-flags.patch
>>>> I don't much like the idea of having flags that have overlapping
bits
>>>> without an obvious reason (0x0020 and 0x0030, for example), but since
>>>> those are pre-existing, I'll leave them alone. Ack.
>>> Can you please elaborate becuase I designed the whole set of flags
to be
>>> overlapping on purpose.
>>> There are three ranges that address three use cases:
>>> 1) Flags that define how to merge values that are encountered
within the
>>> same section
>>> [section]
>>> a=1
>>> a=2
>>> 2) Flags that define how to merge values that are in a section that is
>>> segmented
>>> [section]
>>> a=1
>>> ...
>>> [section]
>>> a=2
>>> 3) Flags that define how to merge sections
>>> But all three are related (especially case 2 & 3) so it makes sense to
>>> pass them in one variable
>>> But if there are some concerns or I am missing something please
explain.
>>
>>
>> #define INI_MV1S_OVERWRITE 0x0000
>> #define INI_MV1S_ERROR 0x0001
>> #define INI_MV1S_PRESERVE 0x0002
>> #define INI_MV1S_ALLOW 0x0003
>> #define INI_MV1S_DETECT 0x0004
>>
>> I don't like that on a bitwise compare, if you do:
>> var = INI_MV1S_ALLOW;
>>
>> then
>>
>> if (var & INI_MV1S_ERROR) {
>> }
>> would be true. This isn't how flags are supposed to work. You're using
>> these more like an enum. It would be better to define them as such (and
>> not call them "flags" if that's what you're trying to do.
>>
>> Calling them flags implies that you would expect to be able to do:
>>
>> var = INI_MV1S_OVERWRITE | INI_MV1S_OVERWRITE;
>>
>> And have both of those be active.
> I view it differently. Within the range they are mutually exclusive but
> they are flags since I expect it to be used in a following way:
> var = INI_MS_MERGE | INI_MV1S_OVERWRITE | INI_MV2S_OVERWRITE;
> Do you have a better way of accomplishing what I am trying to do?
I think this would be better served if var was:
struct ini_struct {
enum ms_cmd ms;
enum mv1s_cmd mv1s;
enum mv2s_cmd mv2s;
}
This would guarantee the mutual exclusivity.
I really do not like this approach. It is too heavy weighted.
I will think more about this though.
May be I will use union and package it all into 32 bit integer.
https://fedorahosted.org/sssd/ticket/767
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel
--
Thank you,
Dmitri Pal
Sr. Engineering Manager IPA project,
Red Hat Inc.
-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/