Hello,
Patch 1: New functionality for refarray. Some basic functionality was missing. Now it is added.
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.
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.
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.
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.
Also, I'd have to check with RFC822 on this, but I think leading spaces followed by ; or # should still be a valid comment.
Please reduce indentation on the mode handling in ini_comment_modify(). It's much too far right. Our coding guideline specifies four spaces.
Otherwise, this looks good to me.
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!
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!
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.
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
On 04/28/2010 01:19 PM, Stephen Gallagher wrote:
Ack.
On 04/28/2010 01:08 PM, Dmitri Pal wrote:
Updated patches attached.
Pushed to master.
sssd-devel@lists.fedorahosted.org
-
Dmitri Pal -
Stephen Gallagher