On Sun, Jan 20, 2002 at 06:06:42PM +0000, Philip Martin wrote:
>...
> Subversion style is to put a space between the function and the
> opening parenthesis [this affects most of the other function calls in
> this patch].
As Garrett pointed out, some files use the space, and others do not. To be
specific, I find the func-space-paren style to be abhorrent and cannot bring
myself to do it. Thus, the files that I've started do not have the space.
>...
> > + const char *key;
> > + char *val;
> > +
> > + apr_hash_this(hi, (const void **)&key, NULL, (void **)&val);
>
> Existing code generally avoids the casts and declares
>
> const void *key;
> apr_ssize_t klen;
> void *val;
> apr_hash_this (hi, &key, &klen, &val);
Agreed. Code that doesn't, should be changed. If a cast is required to
access the key or value, then it should come later. A lot of code will also
do something like:
void *val;
some_struct_t *something;
apr_hash_this(&hi, &key, &klen, &val);
something = val;
whatever(something->foo);
The assignment does the implicit void* -> some_struct_t* cast.
> > +#define NSLEN (sizeof(SVN_PROP_CUSTOM_PREFIX) - 1)
> > + if (strncmp(key, SVN_PROP_CUSTOM_PREFIX, NSLEN) == 0)
> > + apr_hash_set(*props, &key[NSLEN], APR_HASH_KEY_STRING,
> > + svn_string_create(val, ras->pool));
> > +#undef NSLEN
>
> Why the preprocessor abuse? Existing code uses a variable such as
> apr_size_t custom_prefix_len = sizeof (SVN_PROP_CUSTOM_PREFIX) - 1;
Look at the file in question before commenting like that. fetch.c uses the
preprocessor define rather than a variable.
And your later comment about "outguessing the compiler" is misplaced. Just
which compilers will optimize that properly? All of them? We aren't using
only 'gcc', and even then, it will probably do it only with particular
optimization modes, which may be disabled because of a -g switch.
That said, I see it is a stylistic decision (since the performance
difference is so completely moot at this point in the code), so you're free
to use whichever style you want in the code you write.
> However, I worry about the use of strncmp. As far as I can see a user
> can create a property called "custom:xxx" which will erroneously
> compare equal if you use the above test. Now perhaps that should not
> be allowed, but a strcmp, or a klen==custom_prefix_len test, would
> avoid the problem.
As Garrett pointed out, his code is quite fine in this regard. "custom:xxx"
will be named "svn:custom:custom:xxx" over the wire.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Oct 21 14:36:57 2006