Jakub Hrozek wrote:
On 01/21/2010 05:42 AM, Dmitri Pal wrote:
> Hello,
> This patch allows proper parsing of values like this:
> fruit=apple,,orange
> The added function would return the list of three items:
> apple
> <empty>
> orange
> Previously existing parsing functions in this case could only return
> apple
> orange
> This is used in the ELAPI configuration to provide alternative names for
> the column headers.
> This is the last of the non ELAPI patche that I have in my repo.
The only functional problem I see is that the implementation does not
strip leading whitespace, i.e. " foo, bar " would become { " foo",
"bar"
}. Otherwise seems to work fine. Some comments below:
The leading space at the beginning of the value is stripped when value
is read from
key = value
line in the INI file.
And then the logic cleans the spaces around separator so we should be
fine here.
Is there any reason to arbitrarily limit the number of separators?
Also, maybe it would be nicer to use strpbrk() instead of
for(j = 0; j < lensep; j++) {
if(buff[i] == locsep[j])
..one less loop.
I do not see a reason why we should not limit them.
strpbrk definitely has a loop inside so this is a wash.
I think that limited number of separators reminds developers not go
"wild" with the format.
IMO there should be one separator. 3 is a courtesy.
The down side of using strpbrk() is that it is not known which separator
actually matched.
Though this capability is not used I had it in mind for testing purposes.
Changing to strpbrk would cause me to rewrite the logic and retest it.
Do you think it is worth it?
Instead of:
while ((i < dlen) && (isspace(*start)))
maybe you could use:
while (*start && (isspace(*start)))
as the data will be null-terminated strings anyway. That way you could
avoid doing i++ before the loop and i-- immediatelly after it.
I need to adjust
"i" anyways to skip the spaces and not process them
twice so I would have to save initial position and then get delta and
then adjust "i" using delta.
I think in general case you suggestion is correct but in this case it
makes sense to do it as written.
Thank you for review.
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel
--
Thank you,
Dmitri Pal
Engineering Manager IPA project,
Red Hat Inc.
-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/