[SSSD] [PATCHES] New functionality for refarray and new comment object

Dmitri Pal dpal at redhat.com
Tue Apr 27 16:32:56 UTC 2010


Stephen Gallagher wrote:
> On 04/26/2010 04:38 PM, Dmitri Pal wrote:
>   
>> Hello,
>>
>> Patch 1: New functionality for refarray. Some basic functionality was
>> missing. Now it is added.
>>     
>
> Nitpick: in ref_array_replace(), your trace message lists 
> ref_array_insert().  It might not be a bad idea in a separate patch to 
> change TRACE_FLOW_* to use __FUNCTION__ instead of passing it the 
> function name explicitly. It's less error-prone.
>   

Good idea. Did not occur to me earlier.
But It would require going through every function and changing the code
to use the macro.
I suggest we reserve this for the new interfaces and object we add.
So here I will just fix the typo. Ok?


> In ref_array_reset(), you need to document very clearly the order that 
> the callbacks will be invoked or it needs to be documented that the 
> order is not guaranteed. I think the latter is probably a better move, 
> since it grants us leeway in the future, as well encouraging the 
> consumer not to right order-dependent callbacks.
>
>
>   
Same patch or separate patch?



>> Patch 2: New object to preserve and store comments in the INI file.
>> Takes advantage of the refarray interface.
>> This patch includes a separate unit test. Nothing uses this object yet.
>> It will be a part of the new more complex value object I am currently
>> working on.
>>     
>
> I'd prefer ini_comment_is_valid() rather than ini_comment_is_val(). 
> 'val' usually means 'value', so this is confusing.
>   

Ok.

> Also, I'd have to check with RFC822 on this, but I think leading spaces 
> followed by ; or # should still be a valid comment.
>   

So did you check it? If not I can do it myself.

> Please reduce indentation on the mode handling in ini_comment_modify(). 
> It's much too far right. Our coding guideline specifies four spaces.
>   

Sure I will take a look.


> Otherwise, this looks good to me.
>
>
>
>   
Thanks for the review!

-- 
Thank you,
Dmitri Pal

Engineering Manager IPA project,
Red Hat Inc.


-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/




More information about the sssd-devel mailing list