You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by bl...@apache.org on 2012/04/18 23:52:02 UTC
svn commit: r1327695 - /subversion/trunk/subversion/libsvn_subr/checksum.c
Author: blair
Date: Wed Apr 18 21:52:01 2012
New Revision: 1327695
URL: http://svn.apache.org/viewvc?rev=1327695&view=rev
Log:
Refactor svn_checksum_create() to avoid two switch statements.
* subversion/libsvn_subr/checksum.c
(checksum_create):
New static function that creates a svn_checksum_t * with
everything set but the contents of the digest.
(svn_checksum_create):
Use the existing switch statement to get the digest size without
needing DIGESTSIZE(). Use checksum_create().
Modified:
subversion/trunk/subversion/libsvn_subr/checksum.c
Modified: subversion/trunk/subversion/libsvn_subr/checksum.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/checksum.c?rev=1327695&r1=1327694&r2=1327695&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_subr/checksum.c (original)
+++ subversion/trunk/subversion/libsvn_subr/checksum.c Wed Apr 18 21:52:01 2012
@@ -56,25 +56,43 @@ validate_kind(svn_checksum_kind_t kind)
return svn_error_create(SVN_ERR_BAD_CHECKSUM_KIND, NULL, NULL);
}
+/* Create a svn_checksum_t with everything but the contents of the
+ digest populated. */
+static svn_checksum_t *
+checksum_create(svn_checksum_kind_t kind,
+ apr_size_t digest_size,
+ apr_pool_t *pool)
+{
+ // Use apr_palloc() instead of apr_pcalloc() so that the digest
+ // contents are only set once by the caller.
+ svn_checksum_t *checksum = apr_palloc(pool, sizeof(*checksum) + digest_size);
+ checksum->digest = (unsigned char *)checksum + sizeof(*checksum);
+ checksum->kind = kind;
+ return checksum;
+}
svn_checksum_t *
svn_checksum_create(svn_checksum_kind_t kind,
apr_pool_t *pool)
{
svn_checksum_t *checksum;
+ apr_size_t digest_size;
switch (kind)
{
case svn_checksum_md5:
+ digest_size = APR_MD5_DIGESTSIZE;
+ break;
case svn_checksum_sha1:
- checksum = apr_pcalloc(pool, sizeof(*checksum) + DIGESTSIZE(kind));
- checksum->digest = (unsigned char *)checksum + sizeof(*checksum);
- checksum->kind = kind;
- return checksum;
-
+ digest_size = APR_SHA1_DIGESTSIZE;
+ break;
default:
return NULL;
}
+
+ checksum = checksum_create(kind, digest_size, pool);
+ memset((unsigned char *) checksum->digest, 0, digest_size);
+ return checksum;
}
svn_checksum_t *
Re: svn commit: r1327695 - /subversion/trunk/subversion/libsvn_subr/checksum.c
Posted by Blair Zajac <bl...@orcaware.com>.
On 04/18/2012 02:57 PM, Greg Stein wrote:
> On Wed, Apr 18, 2012 at 17:52,<bl...@apache.org> wrote:
>> Author: blair
>> Date: Wed Apr 18 21:52:01 2012
>> New Revision: 1327695
>>
>> URL: http://svn.apache.org/viewvc?rev=1327695&view=rev
>> Log:
>> Refactor svn_checksum_create() to avoid two switch statements.
>>
>> * subversion/libsvn_subr/checksum.c
>> (checksum_create):
>> New static function that creates a svn_checksum_t * with
>> everything set but the contents of the digest.
>> (svn_checksum_create):
>> Use the existing switch statement to get the digest size without
>> needing DIGESTSIZE(). Use checksum_create().
>
> Couldn't you just lose the switch in favor of: digest_size = DIGESTSIZE(kind) ??
That seemed micro-optimization wasteful, as it's checking the kind if
it's a known checksum once and then checking it again on the size.
Blair
Re: svn commit: r1327695 - /subversion/trunk/subversion/libsvn_subr/checksum.c
Posted by Greg Stein <gs...@gmail.com>.
On Wed, Apr 18, 2012 at 17:52, <bl...@apache.org> wrote:
> Author: blair
> Date: Wed Apr 18 21:52:01 2012
> New Revision: 1327695
>
> URL: http://svn.apache.org/viewvc?rev=1327695&view=rev
> Log:
> Refactor svn_checksum_create() to avoid two switch statements.
>
> * subversion/libsvn_subr/checksum.c
> (checksum_create):
> New static function that creates a svn_checksum_t * with
> everything set but the contents of the digest.
> (svn_checksum_create):
> Use the existing switch statement to get the digest size without
> needing DIGESTSIZE(). Use checksum_create().
Couldn't you just lose the switch in favor of: digest_size = DIGESTSIZE(kind) ??
Cheers,
-g
>
> Modified:
> subversion/trunk/subversion/libsvn_subr/checksum.c
>
> Modified: subversion/trunk/subversion/libsvn_subr/checksum.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/checksum.c?rev=1327695&r1=1327694&r2=1327695&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_subr/checksum.c (original)
> +++ subversion/trunk/subversion/libsvn_subr/checksum.c Wed Apr 18 21:52:01 2012
> @@ -56,25 +56,43 @@ validate_kind(svn_checksum_kind_t kind)
> return svn_error_create(SVN_ERR_BAD_CHECKSUM_KIND, NULL, NULL);
> }
>
> +/* Create a svn_checksum_t with everything but the contents of the
> + digest populated. */
> +static svn_checksum_t *
> +checksum_create(svn_checksum_kind_t kind,
> + apr_size_t digest_size,
> + apr_pool_t *pool)
> +{
> + // Use apr_palloc() instead of apr_pcalloc() so that the digest
> + // contents are only set once by the caller.
> + svn_checksum_t *checksum = apr_palloc(pool, sizeof(*checksum) + digest_size);
> + checksum->digest = (unsigned char *)checksum + sizeof(*checksum);
> + checksum->kind = kind;
> + return checksum;
> +}
>
> svn_checksum_t *
> svn_checksum_create(svn_checksum_kind_t kind,
> apr_pool_t *pool)
> {
> svn_checksum_t *checksum;
> + apr_size_t digest_size;
>
> switch (kind)
> {
> case svn_checksum_md5:
> + digest_size = APR_MD5_DIGESTSIZE;
> + break;
> case svn_checksum_sha1:
> - checksum = apr_pcalloc(pool, sizeof(*checksum) + DIGESTSIZE(kind));
> - checksum->digest = (unsigned char *)checksum + sizeof(*checksum);
> - checksum->kind = kind;
> - return checksum;
> -
> + digest_size = APR_SHA1_DIGESTSIZE;
> + break;
> default:
> return NULL;
> }
> +
> + checksum = checksum_create(kind, digest_size, pool);
> + memset((unsigned char *) checksum->digest, 0, digest_size);
> + return checksum;
> }
>
> svn_checksum_t *
>
>
Re: svn commit: r1327695 - /subversion/trunk/subversion/libsvn_subr/checksum.c
Posted by Blair Zajac <bl...@orcaware.com>.
On 04/18/2012 02:54 PM, Greg Stein wrote:
> On Wed, Apr 18, 2012 at 17:52,<bl...@apache.org> wrote:
>> ...
>> +++ subversion/trunk/subversion/libsvn_subr/checksum.c Wed Apr 18 21:52:01 2012
>> @@ -56,25 +56,43 @@ validate_kind(svn_checksum_kind_t kind)
>> return svn_error_create(SVN_ERR_BAD_CHECKSUM_KIND, NULL, NULL);
>> }
>>
>> +/* Create a svn_checksum_t with everything but the contents of the
>> + digest populated. */
>> +static svn_checksum_t *
>> +checksum_create(svn_checksum_kind_t kind,
>> + apr_size_t digest_size,
>> + apr_pool_t *pool)
>> +{
>> + // Use apr_palloc() instead of apr_pcalloc() so that the digest
>> + // contents are only set once by the caller.
>
> No C++ style comments, please...
Oops, thanks. Been coding in C++ too long. Fixed in r1327708.
Blair
RE: svn commit: r1327695 - /subversion/trunk/subversion/libsvn_subr/checksum.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Greg Stein [mailto:gstein@gmail.com]
> Sent: woensdag 18 april 2012 23:55
> To: dev@subversion.apache.org
> Subject: Re: svn commit: r1327695 -
> /subversion/trunk/subversion/libsvn_subr/checksum.c
>
> On Wed, Apr 18, 2012 at 17:52, <bl...@apache.org> wrote:
> >...
> > +++ subversion/trunk/subversion/libsvn_subr/checksum.c Wed Apr 18
> 21:52:01 2012
> > @@ -56,25 +56,43 @@ validate_kind(svn_checksum_kind_t kind)
> > return svn_error_create(SVN_ERR_BAD_CHECKSUM_KIND, NULL, NULL);
> > }
> >
> > +/* Create a svn_checksum_t with everything but the contents of the
> > + digest populated. */
> > +static svn_checksum_t *
> > +checksum_create(svn_checksum_kind_t kind,
> > + apr_size_t digest_size,
> > + apr_pool_t *pool)
> > +{
> > + // Use apr_palloc() instead of apr_pcalloc() so that the digest
> > + // contents are only set once by the caller.
>
> No C++ style comments, please...
This caused build breakage on the Ubuntu gcc buildbot.
See http://subversion.apache.org/buildbot
Bert
>
> >...
>
> Cheers,
> -g
Re: svn commit: r1327695 - /subversion/trunk/subversion/libsvn_subr/checksum.c
Posted by Greg Stein <gs...@gmail.com>.
On Wed, Apr 18, 2012 at 17:52, <bl...@apache.org> wrote:
>...
> +++ subversion/trunk/subversion/libsvn_subr/checksum.c Wed Apr 18 21:52:01 2012
> @@ -56,25 +56,43 @@ validate_kind(svn_checksum_kind_t kind)
> return svn_error_create(SVN_ERR_BAD_CHECKSUM_KIND, NULL, NULL);
> }
>
> +/* Create a svn_checksum_t with everything but the contents of the
> + digest populated. */
> +static svn_checksum_t *
> +checksum_create(svn_checksum_kind_t kind,
> + apr_size_t digest_size,
> + apr_pool_t *pool)
> +{
> + // Use apr_palloc() instead of apr_pcalloc() so that the digest
> + // contents are only set once by the caller.
No C++ style comments, please...
>...
Cheers,
-g