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

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
Received on 2009-03-25 14:35:07 CET