Hyrum K. Wright wrote:
> 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!
Done in r23454.
-Hyrum
Received on Wed Feb 21 17:51:35 2007