Re: svn commit: r36763 - trunk/subversion/libsvn_client

Hi Greg,

Thanks for your exhaustive review, I ve did the changes in r36784.

Greg Stein wrote:
> On Wed, Mar 25, 2009 at 07:33, Senthil Kumaran S <senthil_at_collab.net> wrote:
>> ...
>> +++ trunk/subversion/libsvn_client/add.c Tue Mar 24 23:33:35 2009 (r36763)
>> @@ -82,6 +82,51 @@ trim_string(char **pstr)
>> str[i] = '\0';
>> }
>>
>> +/* Split PROPERTY and store each individual value in PROPS.
>> + Allocates from POOL. */
>> +static void
>> +split_props(apr_array_header_t **props,
>> + const char *property,
>> + apr_pool_t *pool)
>> +{
>> + apr_array_header_t *temp_props;
>> + char *new_prop;
>> + char *prop;
>> + int i = 0;
>> + int j = 0;
>> +
>> + temp_props = apr_array_make(pool, 4, sizeof(const char *));
>> + new_prop = apr_palloc(pool, strlen(property));
>
> allocate strlen(property)+1 .. with the suggested changes below, this
> buffer will be used for all the results, and you may need the +1 to
> store a NUL terminator.
>
>> +
>> + for (i = 0; i <= strlen(property); i++)
>
> for (i = 0; property[i] != '\0'; i++)
>
> would be better: you don't have to compute strlen() every iteration.
>
>> + {
>> + if (property[i] != ';')
>> + {
>> + new_prop[j] = property[i];
>> + j++;
>> + }
>> + else if (property[i] == ';')
>> + {
>> + if (property[i+1] == ';')
>> + {
>> + new_prop[j] = property[i];
>
> new_prop[j] = ';';
>
> we already know its value, so a constant is better.
>
>> + j++;
>> + i++;
>> + }
>> + else
>> + {
>> + new_prop[j] = '\0';
>> + prop = apr_palloc(pool, j+1);
>> + j = 0;
>> + strcpy(prop, new_prop);
>> + APR_ARRAY_PUSH(temp_props, const char *) = prop;
>
> there is no need to make an extra copy. you've allocated modifiable
> memory already. just do the following:
>
> new_prop[j] = '\0';
> APR_ARRAY_PUSH(temp_props, const char *) = new_prop;
> new_prop += j + 1;
> j = 0;
>
>> + }
>> + }
>> + }
>> + APR_ARRAY_PUSH(temp_props, const char *) = new_prop;
>> + *props = temp_props;
>> +}
>> +
>> /* For one auto-props config entry (NAME, VALUE), if the filename pattern
>> NAME matches BATON->filename case insensitively then add the properties
>> listed in VALUE into BATON->properties.
>> @@ -93,9 +138,9 @@ auto_props_enumerator(const char *name,
>> void *baton,
>> apr_pool_t *pool)
>> {
>> + int i;
>> auto_props_baton_t *autoprops = baton;
>> - char *property;
>> - char *last_token;
>> + apr_array_header_t *props;
>>
>> /* nothing to do here without a value */
>> if (strlen(value) == 0)
>> @@ -105,41 +150,44 @@ auto_props_enumerator(const char *name,
>> if (apr_fnmatch(name, autoprops->filename, APR_FNM_CASE_BLIND) == APR_FNM_NOMATCH)
>> return TRUE;
>>
>> - /* parse the value (we dup it first to effectively lose the
>> - 'const', and to avoid messing up the original value) */
>> - property = apr_pstrdup(autoprops->pool, value);
>> - property = apr_strtok(property, ";", &last_token);
>> - while (property)
>> + props = apr_array_make(pool, 4, sizeof(const char *));
>> + split_props(&props, value, autoprops->pool);
>
> No need to allocate an array here -- split_props() creates and returns one.
>
>> +
>> + for (i = 0; i < props->nelts; i++)
>> {
>> int len;
>> + char *new_token;
>
> prop_name, or similar, would be a better name than "new_token"
>
>> const char *this_value;
>> - char *equal_sign = strchr(property, '=');
>> + const char *token = APR_ARRAY_IDX(props, i, const char *);
>> + char *equal_sign = strchr(token, '=');
>>
>> if (equal_sign)
>> {
>> *equal_sign = '\0';
>> equal_sign++;
>> trim_string(&equal_sign);
>> + this_value = apr_palloc(autoprops->pool, strlen(equal_sign)+1);
>> this_value = equal_sign;
>
> what is that allocation for? it gets thrown out by the following line anyways
>
>> }
>> else
>> {
>> this_value = "";
>> }
>> - trim_string(&property);
>> - len = strlen(property);
>> + new_token = apr_pstrdup(autoprops->pool, token);
>
> no need to dup this, it was already allocated in autoprops->pool.
>
>> + trim_string(&new_token);
>> + len = strlen(new_token);
>> +
>> if (len > 0)
>> {
>> svn_string_t *propval = svn_string_create(this_value,
>> autoprops->pool);
>
> No need to copy the contents again. Instead:
>
> svn_string_t *propval = apr_palloc(autoprops->pool, sizeof(*propval));
>
> propval->data = this_value;
> propval->len = strlen(this_value);
>
>> - apr_hash_set(autoprops->properties, property, len, propval);
>> - if (strcmp(property, SVN_PROP_MIME_TYPE) == 0)
>> + apr_hash_set(autoprops->properties, new_token, len, propval);
>> + if (strcmp(new_token, SVN_PROP_MIME_TYPE) == 0)
>> autoprops->mimetype = this_value;
>> - else if (strcmp(property, SVN_PROP_EXECUTABLE) == 0)
>> + else if (strcmp(new_token, SVN_PROP_EXECUTABLE) == 0)
>> autoprops->have_executable = TRUE;
>> }
>> - property = apr_strtok(NULL, ";", &last_token);
>> }
>> return TRUE;
>> }
>>
>> ------------------------------------------------------
>> http://subversion.tigris.org/ds/viewMessage.do?dsForumId=495&dsMessageId=1411351
>>
>
> ------------------------------------------------------
> http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1415500

-- 
Senthil Kumaran S
http://www.stylesen.org/
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1426186