You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@wandisco.com> on 2010/03/18 23:54:46 UTC
Re: svn commit: r924722 - in /subversion/trunk/subversion:
include/svn_wc.h libsvn_client/switch.c libsvn_client/update.c
libsvn_wc/deprecated.c libsvn_wc/update_editor.c
On Thu, 2010-03-18, Greg Stein wrote:
> On Thu, Mar 18, 2010 at 07:34, <ju...@apache.org> wrote:
> > Author: julianfoad
> > Date: Thu Mar 18 11:34:45 2010
> > New Revision: 924722
> >
> > URL: http://svn.apache.org/viewvc?rev=924722&view=rev
> > Log:
> > Clean up two revved WC API function signatures: move the wc_ctx parameter
> > before the first of the main input parameters, to keep those together and
> > to minimize the difference from the previous version. Do some doc string
> > clean-ups too.
> >
> > * subversion/include/svn_wc.h
> > (svn_wc_get_status_editor5): Fix a parameter name typo. Note doc string
> > out-of-dateness.
> > (svn_wc_crawl_revisions5): Note doc string out-of-dateness.
> > (svn_wc_get_update_editor4): Swap revision and wc_ctx parameters.
> > (svn_wc_get_switch_editor4): Swap revision and wc_ctx parameters. Refer to
> > svn_wc_get_update_editor4's doc string for most parameters to make it
> > easy for the reader to see that there are no differences.
>
> Why swap these parameters? REVISION (or TARGET_REVISION) is an OUT
> parameter. We always place OUT parameters first, and then the WC_CTX,
> then the other params.
Bert discussed this with me on IRC too. My log message said why, but
actually there's more to it. I hadn't been able to discern a reason why
the rev-number param was moved away (relative to the previous version of
the API) from the other target parameters in the first place. After
this commit, Bert said he'd done it for the reason you state - output
params come before input params. But this parameter isn't an output
from this function, at least not in the usual sense. It's an address
given to this function, an address for this function to store away but
definitely not to read from or write to. A different function will
later retrieve that address and write a revision number to it.
In that sense, the pointer is an input to this function, whereas in
another sense we could call it an "output pointer" meaning it's a
pointer that will only ever be written through (not read from), but it
is neither a classical "input pointer parameter" nor a classical "output
pointer parameter" for this function. Indeed, the code sequence in the
present only caller of svn_wc_get_update_editor4() depends on this
function call NOT overwriting the pointed-to revision number.
I don't know of a precedent or rule for this specific situation so I
feel either way is OK. Bert said he was happy to call it an input
parameter. If you want to declare that this kind of parameter should be
grouped with classical output pointer parameters, I would be fine with
that decision and would happily change it.
- Julian
Re: svn commit: r924722 - in /subversion/trunk/subversion:
include/svn_wc.h libsvn_client/switch.c libsvn_client/update.c
libsvn_wc/deprecated.c libsvn_wc/update_editor.c
Posted by Julian Foad <ju...@wandisco.com>.
On Thu, 2010-03-18 at 20:21 -0400, Greg Stein wrote:
> On Thu, Mar 18, 2010 at 19:54, Julian Foad <ju...@wandisco.com> wrote:
> > On Thu, 2010-03-18, Greg Stein wrote:
> >> On Thu, Mar 18, 2010 at 07:34, <ju...@apache.org> wrote:
> >> > Author: julianfoad
> >> > Date: Thu Mar 18 11:34:45 2010
> >> > New Revision: 924722
> >> >
> >> > URL: http://svn.apache.org/viewvc?rev=924722&view=rev
> >> > Log:
> >> > Clean up two revved WC API function signatures: move the wc_ctx parameter
> >> > before the first of the main input parameters, to keep those together and
> >> > to minimize the difference from the previous version. Do some doc string
> >> > clean-ups too.
> >> >
> >> > * subversion/include/svn_wc.h
> >> > (svn_wc_get_status_editor5): Fix a parameter name typo. Note doc string
> >> > out-of-dateness.
> >> > (svn_wc_crawl_revisions5): Note doc string out-of-dateness.
> >> > (svn_wc_get_update_editor4): Swap revision and wc_ctx parameters.
> >> > (svn_wc_get_switch_editor4): Swap revision and wc_ctx parameters. Refer to
> >> > svn_wc_get_update_editor4's doc string for most parameters to make it
> >> > easy for the reader to see that there are no differences.
> >>
> >> Why swap these parameters? REVISION (or TARGET_REVISION) is an OUT
> >> parameter. We always place OUT parameters first, and then the WC_CTX,
> >> then the other params.
> >
> > Bert discussed this with me on IRC too. My log message said why, but
> > actually there's more to it. I hadn't been able to discern a reason why
> > the rev-number param was moved away (relative to the previous version of
> > the API) from the other target parameters in the first place. After
> > this commit, Bert said he'd done it for the reason you state - output
> > params come before input params. But this parameter isn't an output
> > from this function, at least not in the usual sense. It's an address
> > given to this function, an address for this function to store away but
> > definitely not to read from or write to. A different function will
> > later retrieve that address and write a revision number to it.
> >
> > In that sense, the pointer is an input to this function, whereas in
> > another sense we could call it an "output pointer" meaning it's a
> > pointer that will only ever be written through (not read from), but it
> > is neither a classical "input pointer parameter" nor a classical "output
> > pointer parameter" for this function. Indeed, the code sequence in the
> > present only caller of svn_wc_get_update_editor4() depends on this
> > function call NOT overwriting the pointed-to revision number.
> >
> > I don't know of a precedent or rule for this specific situation so I
> > feel either way is OK. Bert said he was happy to call it an input
> > parameter. If you want to declare that this kind of parameter should be
> > grouped with classical output pointer parameters, I would be fine with
> > that decision and would happily change it.
>
> Conceptually, it is still an output parameter. You are saying "put the
> result HERE." That's an output parameter.
Output of the editor, not of this function, but OK, that's a fair point
of view. I'll change these two back.
r925061.
- Julian
> Sure, it takes a while to get the value; you have to drive the editor.
> But that's just timing. The concept is still an OUT param.
>
> I certainly have no problem with a redesign of the dataflow, but for
> now... I see it as an OUT parameter.
>
> (note that we have some similar-style params with some
> checksum-computing functions, too)
Re: svn commit: r924722 - in /subversion/trunk/subversion:
include/svn_wc.h libsvn_client/switch.c libsvn_client/update.c
libsvn_wc/deprecated.c libsvn_wc/update_editor.c
Posted by Greg Stein <gs...@gmail.com>.
On Thu, Mar 18, 2010 at 19:54, Julian Foad <ju...@wandisco.com> wrote:
> On Thu, 2010-03-18, Greg Stein wrote:
>> On Thu, Mar 18, 2010 at 07:34, <ju...@apache.org> wrote:
>> > Author: julianfoad
>> > Date: Thu Mar 18 11:34:45 2010
>> > New Revision: 924722
>> >
>> > URL: http://svn.apache.org/viewvc?rev=924722&view=rev
>> > Log:
>> > Clean up two revved WC API function signatures: move the wc_ctx parameter
>> > before the first of the main input parameters, to keep those together and
>> > to minimize the difference from the previous version. Do some doc string
>> > clean-ups too.
>> >
>> > * subversion/include/svn_wc.h
>> > (svn_wc_get_status_editor5): Fix a parameter name typo. Note doc string
>> > out-of-dateness.
>> > (svn_wc_crawl_revisions5): Note doc string out-of-dateness.
>> > (svn_wc_get_update_editor4): Swap revision and wc_ctx parameters.
>> > (svn_wc_get_switch_editor4): Swap revision and wc_ctx parameters. Refer to
>> > svn_wc_get_update_editor4's doc string for most parameters to make it
>> > easy for the reader to see that there are no differences.
>>
>> Why swap these parameters? REVISION (or TARGET_REVISION) is an OUT
>> parameter. We always place OUT parameters first, and then the WC_CTX,
>> then the other params.
>
> Bert discussed this with me on IRC too. My log message said why, but
> actually there's more to it. I hadn't been able to discern a reason why
> the rev-number param was moved away (relative to the previous version of
> the API) from the other target parameters in the first place. After
> this commit, Bert said he'd done it for the reason you state - output
> params come before input params. But this parameter isn't an output
> from this function, at least not in the usual sense. It's an address
> given to this function, an address for this function to store away but
> definitely not to read from or write to. A different function will
> later retrieve that address and write a revision number to it.
>
> In that sense, the pointer is an input to this function, whereas in
> another sense we could call it an "output pointer" meaning it's a
> pointer that will only ever be written through (not read from), but it
> is neither a classical "input pointer parameter" nor a classical "output
> pointer parameter" for this function. Indeed, the code sequence in the
> present only caller of svn_wc_get_update_editor4() depends on this
> function call NOT overwriting the pointed-to revision number.
>
> I don't know of a precedent or rule for this specific situation so I
> feel either way is OK. Bert said he was happy to call it an input
> parameter. If you want to declare that this kind of parameter should be
> grouped with classical output pointer parameters, I would be fine with
> that decision and would happily change it.
Conceptually, it is still an output parameter. You are saying "put the
result HERE." That's an output parameter.
Sure, it takes a while to get the value; you have to drive the editor.
But that's just timing. The concept is still an OUT param.
I certainly have no problem with a redesign of the dataflow, but for
now... I see it as an OUT parameter.
(note that we have some similar-style params with some
checksum-computing functions, too)
Cheers,
-g