Ivan Zhakov wrote:
> On 2/20/07, Hyrum K. Wright <hyrum_wright@mail.utexas.edu> wrote:
>> Ivan Zhakov wrote:
>> > On 2/20/07, Daniel Rall <dlr@collab.net> wrote:
>> >> On Mon, 19 Feb 2007, hwright@tigris.org wrote:
>> >> ...
>> >> > --- trunk/subversion/include/svn_opt.h (original)
>> >> > +++ trunk/subversion/include/svn_opt.h Mon Feb 19 19:45:21
>> 2007
>> >> ...
>> >> > -svn_error_t *
>> >> > -svn_client__resolve_revisions(svn_opt_revision_t *peg_rev,
>> >> > - svn_opt_revision_t *op_rev,
>> >> > - svn_boolean_t is_url,
>> >> > - svn_boolean_t notice_local_mods);
>> >> ...
>> >> > --- trunk/subversion/libsvn_subr/opt.c (original)
>> >> > +++ trunk/subversion/libsvn_subr/opt.c Mon Feb 19 19:45:21
>> 2007
>> >> > @@ -610,6 +610,33 @@
>> >> > }
>> >> >
>> >> >
>> >> > +void
>> >> > +svn_opt_resolve_revisions(svn_opt_revision_t *peg_rev,
>> >> > + svn_opt_revision_t *op_rev,
>> >> > + svn_boolean_t is_url,
>> >> > + svn_boolean_t notice_local_mods)
>> >> > +{
>> >> > + if (peg_rev->kind == svn_opt_revision_unspecified)
>> >> > + {
>> >> > + if (is_url)
>> >> > + {
>> >> > + peg_rev->kind = svn_opt_revision_head;
>> >> > + }
>> >> > + else
>> >> > + {
>> >> > + if (notice_local_mods)
>> >> > + peg_rev->kind = svn_opt_revision_working;
>> >> > + else
>> >> > + peg_rev->kind = svn_opt_revision_base;
>> >> > + }
>> >> > + }
>> >> > +
>> >> > + if (op_rev->kind == svn_opt_revision_unspecified)
>> >> > + *op_rev = *peg_rev;
>> >> > +
>> >> > + return;
>> >> > +}
>> >> ...
>> >>
>> >> If we're really going to drop the return value of SVN_NO_ERROR (which
>> >> we actually might not want to do in case a future variation on the
>> >> implementation needs it), we should drop that return statement, too.
>> >>
>> > Also is there any guarantee that any implementation of this function
>> > can be done without pool parameter in future? I'm not sure.
>> >
>>
>> I'm trying to think of a scenario where we would need a pool parameter
>> in the future. The only thing that I can think of would be if the
>> svn_opt_revision_t structure was extended, and we needed to allocate
>> something to finish the assignment. In this case, though, we'd need to
>> rev the API anyway to use the new datatype.
>>
>> Are there other scenarios where we might need a pool parameter?
> I don't have any specific scenarios now. But it can be just have to
> call some APR function. I remember problems that we had with some
> svn_path_* function, which doesn't have pool parameter and some day we
> was needed call to apr_path_root().
> So personally I prefer each *public* function have pool parameter and
> return svn_error_t, unless we have strong objections.
No strong objections here. In fact, if we return an svn_error_t *,
which I think we should, we'd better be passing in a pool, because we
would need to allocate potential errors from that pool.
I'll make the changes later today. Thanks for the comments!
-Hyrum
Received on Wed Feb 21 14:52:34 2007