On 11/29/2012 05:08 PM, Michal Židek wrote:
On 11/27/2012 03:18 PM, Pavel Březina wrote:
> On 11/26/2012 02:40 PM, Michal Židek wrote:
>> On 11/23/2012 10:11 AM, Pavel Březina wrote:
>>> On 10/17/2012 04:01 PM, Michal Židek wrote:
>>>> On 10/16/2012 11:36 AM, Pavel Březina wrote:
>>>>> On 10/15/2012 12:30 PM, Michal Židek wrote:
>>>>>> Added new parameter to split_on_separator that allows to skip
>>>>>> empty values.
>>>>>>
>>>>>>
https://fedorahosted.org/sssd/ticket/1484
>>>>>>
>>>>>> Patch is in attachment.
>>>>>>
>>>>>> Michal
>>>>>
>>>>> Nack. See below.
>>>>>
>>>>>> --- a/src/util/util.c
>>>>>> +++ b/src/util/util.c
>>>>>> @@ -31,7 +31,8 @@
>>>>>> * the separator is a string, and is case-sensitive.
>>>>>> * optionally single values can be trimmed of of spaces and
>>>>>> tabs */
>>>>>> int split_on_separator(TALLOC_CTX *mem_ctx, const char *str,
>>>>>> - const char sep, bool trim, char
***_list,
>>>>>> int
>>>>>> *size)
>>>>>> + const char sep, bool trim, bool
skip_empty,
>>>>>> + char ***_list, int *size)
>>>>>> {
>>>>>> const char *t, *p, *n;
>>>>>> size_t l, len;
>>>>>> @@ -80,6 +81,11 @@ int split_on_separator(TALLOC_CTX *mem_ctx,
const
>>>>>> char *str,
>>>>>> }
>>>>>>
>>>>>> if (len == 0) {
>>>>>> + if (skip_empty) {
>>>>>> + /* Move to next string and continue */
>>>>>> + t = n;
>>>>>> + continue;
>>>>>> + }
>>>>>
>>>>> Move this^ condition before talloc_realloc.
>>>>>
>>>>
>>>> I did not like the function at all, it seemed to be written for
>>>> different purpose but slightly modified to fit our needs. I wrote a
>>>> new
>>>> version in this patch and added test for it.
>>>>
>>>> New patch attached.
>>>>
>>>> Michal
>>>
>>> Hi,
>>> can you rebase it atop the current master please?
>>>
>>
>> Sure, here is the new patch.
>>
>> Thanks
>> Michal
>
> Thanks.
>
> Nack.
>
> Test issues:
>
>> + {
>> + ", ,,",
>> + (const char*[]){NULL,},
>> + true, true,
>> + 0, 0
>> + },
>
> What is the meaning of the comma after NULL?
>
It had no meaning. Deleted.
>> + for (i = 0; str_ref = sts[a].expected_list[i], str_out =
>> list[i]; i++) {
>> + fail_unless(!strcmp(str_ref, str_out),
>> + "Expected:%s Got:%s\n", str_ref, str_out);
>> + }
>
> Please use strcmp() == 0.
>
> Function issues:
>
>> char **r;
>
> Please, choose a better name for this variable. Something that explains
> its meaning. Maybe 'reallocated_list'?
Done.
>
>> /* Trim leading whitespace if needed */
>> while (trim && isspace(*substr_begin) &&
>> substr_begin < substr_end) {
>> substr_begin++;
>> substr_len--;
>> }
>>
>> /* Trim trailing whitespace if needed */
>> while (trim && substr_end - 1 > substr_begin &&
>> isspace(*(substr_end-1))) {
>> substr_end--;
>> substr_len--;
>> }
>
> Put the condition on one line. And separate trim from while please.
> if (trim) {
> while () // leading
>
> while () // trailing
> }
>
>> if (!(skip_empty && substr_len == 0)) {
>
> if (!skip_empty || substr_len == 0) is more readable in my opinion.
>
Done.
>> if (substr_len == 0) {
>> list[num_strings] = talloc_strdup(list, "");
> ^
>> } else {
>> list[num_strings] = talloc_strndup(list, substr_begin,
>> substr_len);
> ^
>> }
>
> Indentation...
Done.
>
>> talloc_free(list);
>> return ENOMEM;
>
> Can you use "fail" pattern instead of calling the former each time
> allocation fails?
Label "done:" added.
>
> Otherwise it looks good. I like the function.
>
>
New patch attached.
Thanks
Michal
Hi,
I have few more nitpicks.
In tests:
{
NULL,
NULL,
false,false,
^ missing comma :P
0, EINVAL
},
Function:
while (isspace(*substr_begin) &&
substr_begin < substr_end) {
substr_begin++;
substr_len--;
}
/* Trim trailing whitespace */
while (substr_end - 1 > substr_begin &&
isspace(*(substr_end-1))) {
substr_end--;
substr_len--;
}
Can you put those conditions on one line? Both fit into 80 char limit
perfectly and separating them doesn't really help clarity.
reallocated_list = talloc_realloc(mem_ctx, list, char*,
num_strings + 2);
^ indentation :)
and
list = talloc(mem_ctx, char *);
Allocate first on NULL and then talloc_steal() it to mem_ctx. This way
we can catch memory leak. (I know it's obvious there are not any at the
moment, but still...)
And last comment - since the function was completely rewritten, can you
pick either ptr == NULL or !ptr so it is consistent?
I'm sorry, I should have came with these earlier.
Thanks.