On Fri, 17 Feb 2006, Julian Foad wrote:
> Peter N. Lundblad wrote:
>
> One other thing in the log message: it doesn't mention that you changed
> svn_error_codes.h.
>
Woops! Fixed now.
>
> I think the "parse command-line options" function should be at a much higher
> level, at least in libsvn_client and really in "svn" itself.
> Implementation-wise it's trivial, but conceptually a library shouldn't know
> about command-line options, as the concept doesn't apply to other clients.
>
Well, I didn't like the placement entirely either. But I want to use this
function in svnlook as well, so svn or libsvn_client are bad candidates.
libsvn_subr is a bad candidate 'cause that would be a circular dependency
(which would work technically, but still seems odd). So, what's left?
Since this is mostly "compatibility with other diff implementations", I
think it might be fine to have the options parsing convenience function
here.
>
> Other than that, there are just a few documentation nits and a suggestion.
>
> > Index: subversion/include/svn_diff.h
> > ===================================================================
>
> > +typedef struct svn_diff_file_options_t
> > +{
> > + /** To what extent whitespace should be ignored when comparing lines.
> > + * The default is @c svn_diff_file_ignore_space_none. */
>
> It's a bit odd for "The default" to be documented here; this is only a data
> type, and different functions could provide different default values for an
> object of this type.
>
Good point. I'll move this to the _create function.
> > + svn_diff_file_ignore_space_t ignore_space;
> > + /** Whether to treat all end-of-line markers the same when comparing lines.
> > + * The default is @c FALSE. */
> > + svn_boolean_t ignore_eol_style;
>
> I think you might do well to have a set of boolean flags instead of these
> individual fields. It seems likely that we will want to add a few more aspects
> of space-handling behaviour.
>
Maybe, but I choose the enum since these two options are mutually
exclusive (or one is a superset of the other if you want).
> > + * and adjust @a options accordingly. @a options is assumed to be initialized
> > + * with default values. @a pool is used for temporary allocation.
>
> It's not clear here whether "default values" must be the defaults provided by
> options_create() or can be the caller's own choice of defaults. Can we say,
> "assumed to be initialized with valid values," to explicitly allow a caller to
> fill in its own defaults first and then parse options to override some of them.
>
Hmmm... The important thing to bear in mind here is that we only change
the options when we see an existing option in the array. So, if we see
-b, we set that option, but if we don't see -b (or -w) we don't touch the
corresponding field. Do you have a good suggestion for how to phrase this
without writing a novel? :-)
Thanks for the review, JUlian,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Feb 20 20:53:13 2006