On Fri, Nov 29, 2013 at 01:34:27AM -0500, Dmitri Pal wrote:
> On 11/28/2013 03:52 AM, Jakub Hrozek wrote:
>> On Wed, Nov 27, 2013 at 03:24:44PM +0100, Pavel Reichl wrote:
>>> On Tue, 2013-11-26 at 18:25 +0100, Jakub Hrozek wrote:
>>>> On Tue, Nov 26, 2013 at 05:28:18PM +0100, Pavel Reichl wrote:
>>>>> Hi Jakub,
>>>>>
>>>>> thanks for review. I have just a few little remarks, please see
inline.
>>>>>
>>>>> On Tue, 2013-11-26 at 15:23 +0100, Jakub Hrozek wrote:
>>>>>> On Tue, Nov 19, 2013 at 10:14:49PM +0100, Pavel Reichl wrote:
>>>>>>> On Fri, 2013-11-15 at 21:01 +0100, Jakub Hrozek wrote:
>>>>>>>> On Fri, Nov 15, 2013 at 11:15:53AM +0100, Pavel Reichl
wrote:
>>>>>>>>> Hello,
>>>>>>>>>
>>>>>>>>> First patch improves domain detection taking matched
length into account
>>>>>>>>> as proposed and elaborated by Jakub.
>>>>>>>>>
>>>>>>>>> Second patch is a simple unit test testing
sss_ldap_dn_in_search_bases
>>>>>>>>> and sdap_domain_get_by_dn.
>>>>>>>>>
>>>>>>>>> Pavel Reichl
>>>>>>>> Hi Pavel,
>>>>>>>>
>>>>>>>> I haven't really read the full patches yet, just
scrolled through them,
>>>>>>>> so I have only one comment so far:
>>>>>>>>
>>>>>>>>> +++ b/src/tests/cmocka/test_search_bases.c
>>>>>>>>> @@ -0,0 +1,214 @@
>>>>>>>>> +/*
>>>>>>>>> + SSSD
>>>>>>>>> +
>>>>>>>>> + find_uid - Utilities tests
>>>>>>>>> +
>>>>>>>>> + Authors:
>>>>>>>>> + Abhishek Singh
<abhishekkumarsingh.cse(a)gmail.com>
>>>>>>>>> +
>>>>>>>>> + Copyright (C) 2013 Red Hat
>>>>>>>> You should credit yourself :-)
>>>>>>> Hi Jakub,
>>>>>>>
>>>>>>> you are right, thanks for noticing.
>>>>>> Hi, in general the patches are working well and looking good. I
only have
>>>>>> a couple of comments, see inline.
>>>>>>
>>>>>>> From c7632a2310442cfc10708c5728bd63e6a8c916d2 Mon Sep 17
00:00:00 2001
>>>>>>> From: Pavel Reichl <pavel.reichl(a)redhat.com>
>>>>>>> Date: Thu, 14 Nov 2013 21:34:51 +0000
>>>>>>> Subject: [PATCH 1/2] SSSD: Improved domain detection
>>>>>>>
>>>>>>> A bit more elegant way of detection of what domain the group
member belongs to
>>>>>>>
>>>>>>> Resolves:
>>>>>>>
https://fedorahosted.org/sssd/ticket/2132
>>>>>> I only have code style comments about this patch.
>>>>>>
>>>>>>> + tmp_ctx = talloc_new(NULL);
>>>>>>> + if (tmp_ctx == NULL)
>>>>>>> + return NULL;
>>>>>> We use brackets around single-line conditionals as well.
>>>>> OK, I have no trouble fixing this, but accordingly to
>>>>>
http://www.freeipa.org/page/Coding_Style#IF_Statements
>>>>> I got an idea that statements like:
>>>>>
>>>>> if (foo)
>>>>> bar();
>>>>> else
>>>>> baz();
>>>>>
>>>>> are legal so I assumed that:
>>>>>
>>>>> if (foo)
>>>>> bar();
>>>>>
>>>>> would be legal too. So it may be a good idea to clarify this in
above
>>>>> mentioned document or maybe It's just my bad reading. :-)
>>>> Per IPA developers not using braces is discouraged. I agree the wiki
>>>> page should be clarified.
>>>> _______________________________________________
>>>> sssd-devel mailing list
>>>> sssd-devel(a)lists.fedorahosted.org
>>>>
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>>> Hi,
>>>
>>> thanks for information. Improved patches are attached.
>>>
>>> PR
>>>
>> Looks good and works fine.
>>
>> I will just squash in two small style changes I didn't tell you about
>> earlier, but no need to resend the patches:
>>
>> --- a/src/tests/cmocka/test_search_bases.c
>> +++ b/src/tests/cmocka/test_search_bases.c
>> @@ -53,7 +53,7 @@ static struct sdap_search_base** generate_bases(TALLOC_CTX
*mem_ctx,
>> search_bases = talloc_array(mem_ctx, struct sdap_search_base *, n + 1);
>> assert_non_null(search_bases);
>>
>> - for(i=0; i < n; ++i) {
>> + for (i=0; i < n; ++i) {
> I hate to rain on parade but ++i is really confusing.
> I know that for "for" loops it makes no difference (at least this is
> what Goggle said) but for me any use of ++i seems odd.
> It is "increment then do" with languages like C that is 0 based for
> arrays it is more "do and then increment".
> Does the use of ++i (though legitimate in this case) makes only me
> uncomfortable or I am not the only one?
> Can we use i++ and reserve ++i only for the cases where it is really
> meaningful and justified?
> I do not see anything in the style guide about this but should we have
> something?
I'm personally indifferent to this, although I prefer the postfix
notation (i++) as well. But that's just my personal preference.
The reason I brought it up is because when I read the code i++ reads
cleanly while any use of of ++i forces me to stop, pause and drill down
into why this construct is used.
But may be it is just a question pf preference and habit.
In my practice, however, I have seen i++ used about 100 time more
frequently than ++i.
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
--
Thank you,
Dmitri Pal
Sr. Engineering Manager for IdM portfolio
Red Hat Inc.
-------------------------------
Looking to carve out IT costs?