Re: [PATCH] libsvn_wc/props.c misc cleanups

Malcolm Rowe <malcolm-svn-dev@farside.org.uk> writes:
> So here's what I'm thinking of committing. Any comments?

Nice cleanups! Just a few comments:

> Miscellaneous cleanups for libsvn_wc/props.c.
>
> * subversion/libsvn_wc/props.c
> (conflicting_propchanges_p): Rename from
> svn_wc__conflicting_propchanges_p(),
> and make static. Document that DESCRIPTION will be NULL if there
> is no conflict, and make it so.
> (get_existing_prop_reject_file): Rename from
> svn_wc__get_existing_prop_reject_file(), and make static.
> (svn_wc__merge_props): Followup to r17456. Rename local variable
> 'conflict_description' to 'conflict', and remove dependent boolean
> variable 'conflict', as the latter can be derived from the former.
> Update caller of get_existing_prop_reject_file().
> (svn_wc__merge_prop_diffs): Move local variable 'conflict_description'
> into the scope where it's used, and rename to 'conflict',
> again removing a dependent boolean variable. Update callers of
> conflicting_propchanges_p() and get_existing_prop_reject_file().

s/callers of/calls to/

> * subversion/libsvn_wc/props.h
> (svn_wc__conflicting_propchanges_p,
> svn_wc__get_existing_prop_reject_file): Remove prototypes and doc-comments,
> as these functions are now static.

Very minor point: you could just say "Remove, as these are now static."
It goes without saying that when one removes a prototype, one removes
its doc string as well.

> --- subversion/libsvn_wc/props.c (revision 17460)
> +++ subversion/libsvn_wc/props.c (working copy)
> @@ -61,7 +61,7 @@
>
> /* Given two propchange objects, return TRUE iff they conflict. If
> there's a conflict, DESCRIPTION will contain an english description
> - of the problem. */
> + of the problem. Otherwise, *DESCRIPTION will be NULL. */
>
> /* For note, here's the table being implemented:

Heh, this doc string started out with problems: first, that it existed
at all, since (until now) it duplicated the doc string in the header
file. But that's no longer a concern, since the header doc is going
away...

The second problem is that it imprecisely said "DESCRIPTION" instead
of "*DESCRIPTION". Your new text is more precise, but now the doc
string is inconsistent. Can you fix up the previous occurrence too?

(I've decided not to comment on passive voice: "DESCRIPTION will
contain" instead of just "set *DESCRIPTION to". See how good I'm
being, not commenting on that?)

> @@ -271,12 +273,11 @@
> }
>
>
> -/* ### not used outside this file. make it static? */
> -svn_error_t *
> -svn_wc__get_existing_prop_reject_file (const char **reject_file,
> - svn_wc_adm_access_t *adm_access,
> - const char *name,
> - apr_pool_t *pool)
> +static svn_error_t *
> +get_existing_prop_reject_file (const char **reject_file,
> + svn_wc_adm_access_t *adm_access,
> + const char *name,
> + apr_pool_t *pool)

We've lost the doc string from props.h, so we shoul transfer it here.
(All functions should be documented, whether public, semi-public, or
private.)

Everything else looked good to me,
-Karl

-- 
www.collab.net  <>  CollabNet  |  Distributed Development On Demand
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org