Re: svn commit: r14445 - in trunk/subversion: tests/libsvn_subr

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: svn commit: r14445 - in trunk/subversion: tests/libsvn_subr

kfogel
[hidden email] writes:

> --- trunk/subversion/libsvn_subr/path.c (original)
> +++ trunk/subversion/libsvn_subr/path.c Mon Apr 25 15:14:21 2005
> @@ -711,40 +711,35 @@
>  skip_uri_scheme (const char *path)
>  {
>    apr_size_t j;
> -  apr_size_t len = strlen (path);
>  
> -  /* ### Taking strlen() initially is inefficient.  It's a holdover
> -     from svn_stringbuf_t days. */
> +  assert (path);
>  
> -  /* Make sure we have enough characters to even compare. */
> -  if (len < 4)
> +  /* <scheme> has length > 0 and doesn't contain ':' or '/'.
> +     (i.e. the first char is not ':' or '/')
> +     path[1] and path[2] are tested to assert the precondition
> +     of the following loop. */
> +  if (path[0] == '\0' || path[0] == ':' || path[0] == '/'
> +      || path[1] == '\0' || path[2] == '\0')
>      return NULL;
>  
> -  /* Look for the sequence '://' */
> -  for (j = 0; j < len - 3; j++)
> +  for (j = 1; path[j + 2] != '\0'; ++j)
>      {

Still waiting for Greg Hudson to mutter something about cycle
counting... but I confess I enjoyed this code :-).

-K

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: svn commit: r14445 - in trunk/subversion: tests/libsvn_subr

Philip Martin
[hidden email] writes:

> [hidden email] writes:
>> --- trunk/subversion/libsvn_subr/path.c (original)
>> +++ trunk/subversion/libsvn_subr/path.c Mon Apr 25 15:14:21 2005
>> @@ -711,40 +711,35 @@
>>  skip_uri_scheme (const char *path)
>>  {
>>    apr_size_t j;
>> -  apr_size_t len = strlen (path);
>>  
>> -  /* ### Taking strlen() initially is inefficient.  It's a holdover
>> -     from svn_stringbuf_t days. */
>> +  assert (path);
>>  
>> -  /* Make sure we have enough characters to even compare. */
>> -  if (len < 4)
>> +  /* <scheme> has length > 0 and doesn't contain ':' or '/'.
>> +     (i.e. the first char is not ':' or '/')
>> +     path[1] and path[2] are tested to assert the precondition
>> +     of the following loop. */
>> +  if (path[0] == '\0' || path[0] == ':' || path[0] == '/'
>> +      || path[1] == '\0' || path[2] == '\0')
>>      return NULL;
>>  
>> -  /* Look for the sequence '://' */
>> -  for (j = 0; j < len - 3; j++)
>> +  for (j = 1; path[j + 2] != '\0'; ++j)
>>      {
>
> Still waiting for Greg Hudson to mutter something about cycle
> counting... but I confess I enjoyed this code :-).

The problem with code like that (apart from it being difficult to
understand) is that other people always think they can do it better :)

  for(j = 0; path[j]; ++j)
    if (path[j] == ':' || path[j] == '/')
       break;

  if (j > 0 && path[j] == ':' && path[j+1] == '/' && path[j+2] == '/')
    return path + j + 3;

  return NULL;

--
Philip Martin

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: svn commit: r14445 - in trunk/subversion: tests/libsvn_subr

Peter N. Lundblad
In reply to this post by kfogel
On Mon, 25 Apr 2005 [hidden email] wrote:

> [hidden email] writes:
> > --- trunk/subversion/libsvn_subr/path.c (original)
> > +++ trunk/subversion/libsvn_subr/path.c Mon Apr 25 15:14:21 2005
> > @@ -711,40 +711,35 @@
> >  skip_uri_scheme (const char *path)
> >  {
> >    apr_size_t j;
> > -  apr_size_t len = strlen (path);
> >
> > -  /* ### Taking strlen() initially is inefficient.  It's a holdover
> > -     from svn_stringbuf_t days. */
> > +  assert (path);
> >
> > -  /* Make sure we have enough characters to even compare. */
> > -  if (len < 4)
> > +  /* <scheme> has length > 0 and doesn't contain ':' or '/'.
> > +     (i.e. the first char is not ':' or '/')
> > +     path[1] and path[2] are tested to assert the precondition
> > +     of the following loop. */
> > +  if (path[0] == '\0' || path[0] == ':' || path[0] == '/'
> > +      || path[1] == '\0' || path[2] == '\0')
> >      return NULL;
> >
> > -  /* Look for the sequence '://' */
> > -  for (j = 0; j < len - 3; j++)
> > +  for (j = 1; path[j + 2] != '\0'; ++j)
> >      {
>
> Still waiting for Greg Hudson to mutter something about cycle
> counting... but I confess I enjoyed this code :-).
>
Yeah, and I waited for a comment like that:-) Note that it was a patch; I
wouldn't have spent time on it except this.


(Sometimes I think it is good to apply patches even if they are very smal
improvements. Maybe it stimulates someone ore than closing an issue as
WONTFIX.)

Regards,
//Peter

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Loading...