danderson@tigris.org writes:
> --- trunk/subversion/libsvn_ra/ra_loader.c (original)
> +++ trunk/subversion/libsvn_ra/ra_loader.c Tue Aug 30 18:58:16 2005
> @@ -230,6 +230,14 @@
> return SVN_NO_ERROR;
> }
>
> +svn_error_t *
> +svn_ra_create_callbacks (svn_ra_callbacks2_t **callbacks,
> + apr_pool_t *pool)
> +{
> + *callbacks = apr_pcalloc (pool, sizeof (svn_ra_callbacks2_t));
> + return SVN_NO_ERROR;
> +}
I know this is going to seem picky, but we typically do:
*callbacks = apr_pcalloc (pool, sizeof (**callbacks));
so the type can change without the code needing to be updated.
> svn_error_t *svn_ra_open2 (svn_ra_session_t **session_p,
> const char *repos_URL,
> const svn_ra_callbacks2_t *callbacks,
> @@ -290,17 +298,18 @@
> {
> /* Deprecated function. Copy the contents of the svn_ra_callbacks_t
> to a new svn_ra_callbacks2_t and call svn_ra_open2(). */
> - svn_ra_callbacks2_t callbacks2;
> - callbacks2.open_tmp_file = callbacks->open_tmp_file;
> - callbacks2.auth_baton = callbacks->auth_baton;
> - callbacks2.get_wc_prop = callbacks->get_wc_prop;
> - callbacks2.set_wc_prop = callbacks->set_wc_prop;
> - callbacks2.push_wc_prop = callbacks->push_wc_prop;
> - callbacks2.invalidate_wc_props = callbacks->invalidate_wc_props;
> - callbacks2.progress_func = NULL;
> - callbacks2.progress_baton = NULL;
> + svn_ra_callbacks2_t *callbacks2;
> + SVN_ERR (svn_ra_create_callbacks (callbacks2, pool));
> + callbacks2->open_tmp_file = callbacks->open_tmp_file;
> + callbacks2->auth_baton = callbacks->auth_baton;
> + callbacks2->get_wc_prop = callbacks->get_wc_prop;
> + callbacks2->set_wc_prop = callbacks->set_wc_prop;
> + callbacks2->push_wc_prop = callbacks->push_wc_prop;
> + callbacks2->invalidate_wc_props = callbacks->invalidate_wc_props;
> + callbacks2->progress_func = NULL;
> + callbacks2->progress_baton = NULL;
I just want you to know that I *did* notice that you need to pass
"&callbacks2" up there :-). But then I noticed a commit right from
you right after this one... that seemed promising, took a look, saw
that r16011 fixes this :-).
> --- trunk/subversion/libsvn_ra/wrapper_template.h (original)
> +++ trunk/subversion/libsvn_ra/wrapper_template.h Tue Aug 30 18:58:16 2005
> @@ -45,20 +45,21 @@
> apr_pool_t *pool)
> {
> svn_ra_session_t *sess = apr_pcalloc (pool, sizeof (svn_ra_session_t));
> - svn_ra_callbacks2_t callbacks2;
> sess->vtable = &VTBL;
> sess->pool = pool;
> + svn_ra_callbacks2_t *callbacks2;
> + SVN_ERR (svn_ra_create_callbacks (callbacks2, pool));
Same here of course.
> --- trunk/subversion/libsvn_ra_dav/session.c (original)
> +++ trunk/subversion/libsvn_ra_dav/session.c Tue Aug 30 18:58:16 2005
> @@ -561,14 +561,23 @@
> #endif /* if/else SVN_NEON_0_25 */
> }
>
> +typedef struct neonprogress_baton_t
> +{
> + void *progress_baton;
> + svn_ra_progress_notify_func_t progress_func;
> + apr_pool_t *pool;
> +} neonprogress_baton_t;
I think we typically don't typedef internal structures like this, we
save that for the public APIs. Not worth changing though, just
mentioning it.
You might something simple like this doesn't need documentation, but
(for exampl) I'm not sure what the pool parameter is for, and whether
it has any relationship to the lifetime of the structure as a whole or
of the baton inside the structure. Presumably not, but explanations
of the structure as a whole and of the fields help clear up such
questions before they become an issue.
At the very least, say "/* The baton type used by ra_dav_neonprogress(). */".
> static void
> -ra_dav_neonprogress(void * baton, off_t progress, off_t total)
> +ra_dav_neonprogress(void *baton, off_t progress, off_t total)
> {
> - /* Important: don't change the svn_ra_callbacks2_t struct here! */
> - const svn_ra_callbacks2_t * callbacks = (svn_ra_callbacks2_t *)baton;
> - if (callbacks->progress_func)
> + const neonprogress_baton_t *neonprogress_baton =
> + (neonprogress_baton_t *)baton;
> + if (neonprogress_baton->progress_func)
> {
> - callbacks->progress_func(progress, total, callbacks->progress_baton);
> + neonprogress_baton->progress_func(progress, total,
> + neonprogress_baton->progress_baton,
> + neonprogress_baton->pool);
> }
> }
>
> @@ -593,6 +602,8 @@
> svn_boolean_t compression;
> svn_config_t *cfg;
> const char *server_group;
> + neonprogress_baton_t *neonprogress_baton =
> + apr_pcalloc(pool, sizeof(neonprogress_baton_t));
"sizeof(*neonprogress_baton)"
> /* Sanity check the URI */
> if (ne_uri_parse(repos_URL, &uri)
> @@ -802,13 +813,11 @@
> ne_ssl_trust_default_ca(sess2);
> }
> }
> -
> - /* Set the neon callback to make it call the svn_progress_notify_func_t
> - * Note that ne_set_progress() takes a non-const baton as the third param.
> - * Since we don't change the callback struct but only use the non-const
> - * notification callback items of that struct, it's safe to cast */
> - ne_set_progress(sess, ra_dav_neonprogress, (void*)callbacks);
> - ne_set_progress(sess2, ra_dav_neonprogress, (void*)callbacks);
> + neonprogress_baton->pool = pool;
> + neonprogress_baton->progress_baton = callbacks->progress_baton;
> + neonprogress_baton->progress_func = callbacks->progress_func;
> + ne_set_progress(sess, ra_dav_neonprogress, (void*)neonprogress_baton);
> + ne_set_progress(sess2, ra_dav_neonprogress, (void*)neonprogress_baton);
> session->priv = ras;
Okay, here it turns out that the pool in which neonprogress_baton is
allocated is also the pool it stores. That's okay, but maybe the
field's documentation in the structure should say that this can be the
case.
Are the (void*) casts really necessary? Shouldn't it compile fine
without them? I realize they were there already, but they seem like
noise to me.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Sep 17 00:02:54 2005