Ack. On 04/28/2010 01:08 PM, Dmitri Pal wrote: > Stephen Gallagher wrote: >> On 04/27/2010 12:32 PM, Dmitri Pal wrote: >> >>> 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? >>> >>> >> >> As I said, updating that macro should be a future patch. Just fix the >> typo for now. >> >> >>>> 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? >>> >> >> Please amend this 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. >>> >> >> Actually, I withdraw this. Leading spaces in RFC 822 are only allowed >> when denoting multi-line values >> >> >>>> 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! >>> >>> >> >> >> > Updated patches attached. > > > > > _______________________________________________ > sssd-devel mailing list > sssd-devel@lists.fedorahosted.org > https://fedorahosted.org/mailman/listinfo/sssd-devel
-- Stephen Gallagher RHCE 804006346421761 Delivering value year after year. Red Hat ranks #1 in value among software vendors. http://www.redhat.com/promo/vendor/ _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
