lundblad@tigris.org writes:
> --- trunk/subversion/libsvn_subr/utf.c (original)
> +++ trunk/subversion/libsvn_subr/utf.c Sat Oct 30 17:17:58 2004
> @@ -55,6 +55,15 @@
> apr_xlate_t *handle;
> struct xlate_handle_node_t *next;
> } xlate_handle_node_t;
> +
> +/* This maps userdata_key strings to pointers to pointers to the first entry
> + in the linked list of xlate handles.
> + We don't store the pointer to the list head directly in the hash table,
> + since we remove/insert entries at the head in the list in the code below,
> + and we can't use apr_hash_set() in each character translation because that
> + function allocates memory in each call where the value is non-NULL.
> + Since these allocations take place in a global pool, this would be a
> + memory leak. */
> static apr_hash_t *xlate_handle_hash = NULL;
I think it's better to document with actual types, that is, the first
part of this doc string would be something like:
/* This maps 'char *' userdata_key strings to 'xlate_handle_node_t **'
* handles to the first entry in the linked list of xlate handles.
* ...
*/
The exact wording isn't important, just as long as we get the C types
there.
I like the rest of the change, by the way -- the code feels easier to
read this way. One totally nitpicking comment, not about this commit:
in get_xlate_handle_node() we have
(**ret).next = NULL;
This is the only place where the ".next" syntax is used. Is there
some reason not to do
(*ret)->next = NULL;
?
The only reason I ask is that someone might do a search for "->next".
(Note that a similar question applies to the line above it.)
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Nov 1 17:35:47 2004