Karl Fogel <kfogel@newton.ch.collab.net> writes:
> 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.
A reasonable point I'll conceed. I'll refine my comment to "It's not
the way I'd write it if I was starting from scratch". Why? Well seeing
char buffer[5];
in one file and
sprintf(ptr, "%c%c%c%c", ...);
in a different file, makes me uncomfortable. It was less of a problem
when it was confined to one file.
--
Philip
---------------------------------------------------------------------
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