Philip Martin <philip@codematters.co.uk> writes:
> > /* Fill in the first four characters of STR_STATUS with status code
> > - characters, based on TEXT_STATUS, PROP_STATUS, LOCKED, and COPIED. */
> > -static void
> > -generate_status_codes (char *str_status,
> > - enum svn_wc_status_kind text_status,
> > - enum svn_wc_status_kind prop_status,
> > - svn_boolean_t locked,
> > - svn_boolean_t copied)
> > + characters, based on TEXT_STATUS, PROP_STATUS, LOCKED, and COPIED.
> > +
> > + This function is also used by commit-cmd.c
> > +*/
> > +void
> > +svn_cl__generate_status_codes (char *str_status,
>
> This is a nasty interface for a function called across files. You need
> to find every caller if you need to change how much gets written to
> str_status. I think the function should be allocating the memory (an
> svn_string_t perhaps), or the length of the array should be passed in,
> or the function should write directly to a file.
Is it really so nasty? The point is that the callers need to depend
things lining up in columns. And if this function were ever going to
print more than four chars into STR_STATUS, it would be taking more
arguments too, so all those callers would have to change anyway.
Personally, I don't see a problem with this interface.
Completely agree with all your other comments, Philip, and thanks so
much for doing the review!
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Oct 21 14:36:59 2006