lundblad@tigris.org writes:
> Author: lundblad
> Date: Thu Jan 20 15:05:59 2005
> New Revision: 12801
>
> Log:
> New RA API.
So Peter, I'm not going to send my review all at once -- instead I'll
ask questions in multiple mails. (I'm not done with the review yet
anyway, but even if I were, multiple mails might be easier.)
> --- trunk/subversion/include/svn_ra.h (original)
> +++ trunk/subversion/include/svn_ra.h Thu Jan 20 15:05:59 2005
> @@ -277,16 +277,551 @@
>
> /*----------------------------------------------------------------------*/
>
> -/** The RA Library.
> +/* Public Interfaces. */
> +
> +/**
> + * @since New in 1.2.
> + *
> + * A repository access session. This object is used to perform requests
> + * to a repository, identified by an URL.
> + */
> +typedef struct svn_ra_session_t svn_ra_session_t;
> +
> +/**
> + * @since New in 1.2.
> + *
> + * Open a repository session to @a repos_url. Return an opaque object
> + * representing this session in @a *session_p, allocated in @a pool.
> + *
> + * @a callbacks/@a callback_baton is a table of callbacks provided by the
> + * client; see @c svn_ra_callbacks_t above.
> + *
> + * @a config is a hash mapping <tt>const char *</tt> keys to
> + * @c svn_config_t * values. For example, the @c svn_config_t for the
> + * "~/.subversion/config" file is under the key "config".
> + *
> + * All RA requests require a session; they will continue to
> + * use @a pool for memory allocation.
> + */
> +svn_error_t *svn_ra_open (svn_ra_session_t **session_p,
> + const char *repos_URL,
> + const svn_ra_callbacks_t *callbacks,
> + void *callback_baton,
> + apr_hash_t *config,
> + apr_pool_t *pool);
This doc string says that RA requests will "continue to use @a pool
for memory allocation", referring the pool parameter above, which was
passed to svn_ra_open() and presumably is preserved in the session
object.
But this seems to contradict the doc strings of the svn_ra_* functions
themselves. For examples:
> +/**
> + * @since New in 1.2.
> + *
> + * Get the latest revision number from the repository of @a session.
> + *
> + * Use @a pool for memory allocation.
> + */
> +svn_error_t *svn_ra_get_latest_revnum (svn_ra_session_t *session,
> + svn_revnum_t *latest_revnum,
> + apr_pool_t *pool);
Okay, so which pool is being used for memory allocation again? :-)
> +/**
> + * @since New in 1.2.
> + *
> + * Get the latest revision number at time @a tm in the repository of
> + * @a session.
> + *
> + * Use @a pool for memory allocation.
> + */
> +svn_error_t *svn_ra_get_dated_revision (svn_ra_session_t *session,
> + svn_revnum_t *revision,
> + apr_time_t tm,
> + apr_pool_t *pool);
Same...
> +/**
> + * @since New in 1.2.
> + *
> + * Set the property @a name to @a value on revision @a rev in the repository
> + * of @a session.
> + *
> + * If @a value is @c NULL, delete the named revision property.
> + *
> + * Please note that properties attached to revisions are **unversioned**.
> + *
> + * Use @a pool for memory allocation.
> + */
> +svn_error_t *svn_ra_change_rev_prop (svn_ra_session_t *session,
> + svn_revnum_t rev,
> + const char *name,
> + const svn_string_t *value,
> + apr_pool_t *pool);
Same...
Well, you get the idea, I won't list every function here.
Callers that plan to call svn_ra_* functions in loops will want to
know if they should use the usual loop-pool idiom. I suspect the
answer is "yes, they should". So I think the way to resolve this is
for the docstring for svn_ra_open() to explain in detail how that pool
will actually be used.
But:
If that initial pool is used for much beyond the allocation of the
session object, then won't we have a memory leak problem when ra
functions are called repeatedly in loops? Because even if the caller
dutifully passes a reuseable pool to each svn_ra_foo() call, some
allocation would still be happening in that original pool, beyond the
control of the calling loop.
(I haven't looked at the implementations yet, so maybe this isn't a
concern?)
Anyway, that's my first question. Review will continue for several
days; I'm planning to take the printout of the commit on the plane
with me to California on Monday, in fact.
Best,
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Jan 22 08:50:28 2005