I trust this provision of copy functions is the best way to solve the problems
that the bindings are having? Can anyone see these functions being useful in
general, or just for this particular case?
Please excuse the long-windedness of the rest of this message. Essentially the
patch looks fine. I have only stylistic requests, and the most significant of
those is moving one function to a different file.
Naming:
We have svn_create_commit_info(), so shouldn't we have "svn_dup_commit_info()"
etc.? I think not: I think that names beginning with a prefix that identifies
the library they belong to are good, and the name starting "svn_create" is bad.
(It's a funny one, the only one like it, and one that doesn't seem to belong
to a particular library.)
We also have:
svn_xml_parser_t * svn_xml_make_parser
svn_wc_traversal_info_t *svn_wc_init_traversal_info
svn_string[buf]_t * svn_string[buf]_create
svn_ra_svn_conn_t * svn_ra_svn_create_conn
svn_fs_t * svn_fs_new
svn_wc_notify_t * svn_wc_create_notify
svn_wc_notify_t * svn_wc_dup_notify
svn_wc_status2_t * svn_wc_dup_status2
svn_wc_entry_t * svn_wc_entry_dup
svn_error_t * svn_error_dup
svn_string[buf]_t * svn_string[buf]_dup
svn_lock_t * svn_lock_dup
So, only a little consistency to guide us.
I think these new names are OK (ending in "_dup").
Which source file:
I think "libsvn_subr/constructors.c" is only appropriate for constructors that
don't have a better home. That includes svn_log_changed_path_dup(), but I
think svn_auth_ssl_server_cert_info_dup() should go in auth.c.
C. Michael Pilato wrote:
> David James <james82@gmail.com> writes:
>>Create copy constructors for svn_txdelta_window_t,
>>svn_log_changed_path_t, and svn_auth_ssl_server_cert_info_t.
>>(These copy constructors are needed to solve the segfaults
>> in the Python bindings caused by issue 2172.)
[...]
> [...] Something like:
>
> Fix segfaults in the Python bindings caused by pool lifetime
> problems by creating/exposing deep-copy utilities for various
> structure types. This is a followup to the work done for issue
> #2172.
Except that this patch doesn't fix anything in the Python bindings. :-)
David's summary was well written, emphasising what the patch does, with a
parenthetical note about why it was done. Certainly "Create" would be more
accurate as "Create or expose", and replacing the C++ language "copy
constructors" with a more universal phrase like "deep-copy utilities" is also
fine, and the other changes C-Mike mentioned.
Some source formatting style nits which we could easily fix up while committing
the patch:
> Index: subversion/include/svn_delta.h
[...]
> +svn_txdelta_window_t *svn_txdelta_window_dup (
> + const svn_txdelta_window_t *window, apr_pool_t *pool);
This would be better if you start the function name on a new line, as we often
do especially when the return type is long, like this:
svn_txdelta_window_t *
svn_txdelta_window_dup (const svn_txdelta_window_t *window,
apr_pool_t *pool);
> Index: subversion/include/svn_auth.h
[...]
> +svn_auth_ssl_server_cert_info_t *svn_auth_ssl_server_cert_info_dup (
> + const svn_auth_ssl_server_cert_info_t *info, apr_pool_t *pool);
And here.
> Index: subversion/libsvn_subr/constructors.c
[...]
> + svn_auth_ssl_server_cert_info_t *new_info =
> + apr_palloc (pool, sizeof (*new_info));
We tend not to leave an operator (like "=" or "+") on the end of a line; move
it to the beginning of the next line (indented by two spaces). (Here and in
two more places.)
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Sep 29 01:26:51 2005