You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by "Peter N. Lundblad" <pe...@famlundblad.se> on 2006/01/28 22:03:26 UTC
[API] r18266
Julian,
In r18266, you added a new API svn_wc_revision_status with a pointer to
struct svn_wc_revision_status_t, which will be filled by the function.
This avoids lots of "out" parameters in the function, but it still makes
it impossible to add new fields to the struct without revving both the
struct and the function (which might be another reason for using a struct
in the first place).
I suggest making the function allocate the struct and adding to the struct
doc a note that the size of this struct may increase in the future, so
callers shouldn't create it themselves. (We can add constructors like
we've done for other structs now or on demand later).
We've made this mistake before leading to lots of API revision.
Thoughts?
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [API] r18266
Posted by Julian Foad <ju...@btopenworld.com>.
Peter N. Lundblad wrote:
> Julian,
>
> In r18266, you added a new API svn_wc_revision_status with a pointer to
> struct svn_wc_revision_status_t, which will be filled by the function.
> This avoids lots of "out" parameters in the function, but it still makes
> it impossible to add new fields to the struct without revving both the
> struct and the function (which might be another reason for using a struct
> in the first place).
>
> I suggest making the function allocate the struct and adding to the struct
> doc a note that the size of this struct may increase in the future, so
> callers shouldn't create it themselves. (We can add constructors like
> we've done for other structs now or on demand later).
>
> We've made this mistake before leading to lots of API revision.
That sounds like a good idea. Anyone willing to do a patch for it? (It wasn't
really my patch, but since I worked on it and committed it I suppose I ought to
take more than average responsibility for it, so I'll try to take a look at
this some time if nobody else offers.)
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [API] r18266
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Sat, 4 Feb 2006, Greg Stein wrote:
> On Sat, Feb 04, 2006 at 10:57:34PM +0100, Peter N. Lundblad wrote:
> > On Sat, 4 Feb 2006, Greg Stein wrote:
> > > On Sat, Feb 04, 2006 at 10:31:55PM +0100, Peter N. Lundblad wrote:
> > > >...
> > > > Hrm!?! Some time ago, I revised the notification API to use a struct
> > > > instead of lots of parameters just so we could add more in the future if
> > >
> > > For a notification API, you're the only one allocating the structure.
> > > Stating a contract that a client shouldn't allocate it is fine. If a
> > > structure needs to be alloc'd by a client, then a factory function is
> > > best (per my previous email).
> >
> > We're discussing forwards-compatibility here, i.e. compiling against 1.4
> > and dynamically linking against an 1.3 library. If we'd support that, we'd
> > not allow extension of structures (or enums in some situations) because
> > the caller would read beyond the returned struct.
>
> I don't get it. That situation is NEVER allowed.
>
> You can compile against 1.3 and dynlink against 1.4. But NOT the other
> way around.
>
Then *we* agree... maxb doesn't seem to. It's him you should argue with,
not me:-)
Thanks,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [API] r18266
Posted by Greg Stein <gs...@lyra.org>.
On Sat, Feb 04, 2006 at 10:57:34PM +0100, Peter N. Lundblad wrote:
> On Sat, 4 Feb 2006, Greg Stein wrote:
> > On Sat, Feb 04, 2006 at 10:31:55PM +0100, Peter N. Lundblad wrote:
> > >...
> > > Hrm!?! Some time ago, I revised the notification API to use a struct
> > > instead of lots of parameters just so we could add more in the future if
> >
> > For a notification API, you're the only one allocating the structure.
> > Stating a contract that a client shouldn't allocate it is fine. If a
> > structure needs to be alloc'd by a client, then a factory function is
> > best (per my previous email).
>
> We're discussing forwards-compatibility here, i.e. compiling against 1.4
> and dynamically linking against an 1.3 library. If we'd support that, we'd
> not allow extension of structures (or enums in some situations) because
> the caller would read beyond the returned struct.
I don't get it. That situation is NEVER allowed.
You can compile against 1.3 and dynlink against 1.4. But NOT the other
way around.
Going back to your first post, you're talking about the "good"
direction. So am I missing something else? (and yes, your original
suggestion of function-allocates-and-returns seems appropriate)
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [API] r18266
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Sat, 4 Feb 2006, Greg Stein wrote:
> On Sat, Feb 04, 2006 at 10:31:55PM +0100, Peter N. Lundblad wrote:
> >...
> > Hrm!?! Some time ago, I revised the notification API to use a struct
> > instead of lots of parameters just so we could add more in the future if
>
> For a notification API, you're the only one allocating the structure.
> Stating a contract that a client shouldn't allocate it is fine. If a
> structure needs to be alloc'd by a client, then a factory function is
> best (per my previous email).
>
We're discussing forwards-compatibility here, i.e. compiling against 1.4
and dynamically linking against an 1.3 library. If we'd support that, we'd
not allow extension of structures (or enums in some situations) because
the caller would read beyond the returned struct.
> > needed. The reason was to add some field when we implemented locking. If
> > you call that a "little API revving", then go look for that diff...
> >
> > Also, what about our version check macros? Don't they protect applications
> > from such badness?
>
> If the client uses them, then they can help. But they protect against
> going backwards in time, or major revision changes. Increasing minor
> revs are fine, so if you change a struct size across a minor rev, then
> the check macros aren't going to say anything (since that is defined
> as a legal version change).
That's clear. That's why client-allocatable structs are bad, bad, bad.
Thanks,
//P
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [API] r18266
Posted by Greg Stein <gs...@lyra.org>.
On Sat, Feb 04, 2006 at 10:31:55PM +0100, Peter N. Lundblad wrote:
>...
> Hrm!?! Some time ago, I revised the notification API to use a struct
> instead of lots of parameters just so we could add more in the future if
For a notification API, you're the only one allocating the structure.
Stating a contract that a client shouldn't allocate it is fine. If a
structure needs to be alloc'd by a client, then a factory function is
best (per my previous email).
> needed. The reason was to add some field when we implemented locking. If
> you call that a "little API revving", then go look for that diff...
>
> Also, what about our version check macros? Don't they protect applications
> from such badness?
If the client uses them, then they can help. But they protect against
going backwards in time, or major revision changes. Increasing minor
revs are fine, so if you change a struct size across a minor rev, then
the check macros aren't going to say anything (since that is defined
as a legal version change).
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [API] r18266
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Sat, 4 Feb 2006, Max Bowsher wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Julian Foad wrote:
> > Peter N. Lundblad wrote:
> >> On Thu, 2 Feb 2006, Julian Foad wrote:
> >>> Peter N. Lundblad wrote:
> >>>
> >>>> I don't understand the above. If we forbid users to allocate this
> >>>> struct,
> >>>> we can freely add fields in the future without problems. Or do you
> >>>> mean
> >>>> that we have a rule that if you compile with library x+1, and it links
> >>>> with lbirary x, it should work? Is that what you mean by "forward
> >>>> compatibility"?
> >>>
> >>> Yes, that's what I meant.
> >>
> >> Is that really something we need to care about? If you link against
> >> 1.4.17
> >> and then, at runtime, use 1.3.4, could you expect that to work, then? I
> >> don't think we leave such guarantees.
> >
> > Ah, yes, you're right. An application for 1.4.x libraries is not
> > expected to work when run with 1.3.x libraries, so we can add extra
> > fields in the next minor version, and don't care that an application
> > using them won't work if run with older libraries.
> >
> > The difference from our normal API compatibility process is that in this
> > case the incompatibility won't be detected at link time. If a
> > particular application uses some features of the new library, but only
> > ones of this kind, then it will load and start to run against old
> > libraries but with undefined behaviour. I don't know whether we want to
> > care about that.
>
> Ouch! Even if this is permissible by a strict semantic reading of the
> compat rules, do we really want to create such a situation?
>
> Currently, the situation is that any compatibility problem will sooner
> or later trigger some kind of symbol-not-found error - a situation which
> allows for moderately graceful failure.
>
> I don't think it does either us, or the clients of our libraries, to
> avoid a little API revving if the cost is the potential for obscure and
> hard-to-detect bugs. I suggest simply contracting to rev the API
> whenever the structure changes (it's not that hard, after all).
>
Hrm!?! Some time ago, I revised the notification API to use a struct
instead of lots of parameters just so we could add more in the future if
needed. The reason was to add some field when we implemented locking. If
you call that a "little API revving", then go look for that diff...
Also, what about our version check macros? Don't they protect applications
from such badness?
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [API] r18266
Posted by Greg Stein <gs...@lyra.org>.
On Sat, Feb 04, 2006 at 10:23:16PM +0100, Branko ??ibej wrote:
>...
> >>>>Peter N. Lundblad wrote:
> >>>>>I don't understand the above. If we forbid users to allocate this
> >>>>>struct,
> >>>>>we can freely add fields in the future without problems. Or do you
We've stated this before around structures. We've also moved to use
more factory functions so that we don't always have to make such
statements (which are hard to enforce).
>...
> >I don't think it does either us, or the clients of our libraries, to
> >avoid a little API revving if the cost is the potential for obscure and
> >hard-to-detect bugs. I suggest simply contracting to rev the API
> >whenever the structure changes (it's not that hard, after all).
It gets ugly to keep creating foo2, foo3, ...
> We have version checks that enforce the compatibility rules.
Only for well-behaved clients that call those functions. The version
rules were also designed to make it "just work" for any client.
In any case, a strict reading of the version rules would forbid adding
fields to a structure. But it is also legal to say "don't alloc these
yourself" and that escapes the API rules. I don't have any opinion
here, but would tend to recommend factory functions and mandate
clients use those rather than their own allocations (on heap or stack).
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [API] r18266
Posted by Branko Čibej <br...@xbc.nu>.
Max Bowsher wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Julian Foad wrote:
>
>> Peter N. Lundblad wrote:
>>
>>> On Thu, 2 Feb 2006, Julian Foad wrote:
>>>
>>>> Peter N. Lundblad wrote:
>>>>
>>>>
>>>>> I don't understand the above. If we forbid users to allocate this
>>>>> struct,
>>>>> we can freely add fields in the future without problems. Or do you
>>>>> mean
>>>>> that we have a rule that if you compile with library x+1, and it links
>>>>> with lbirary x, it should work? Is that what you mean by "forward
>>>>> compatibility"?
>>>>>
>>>> Yes, that's what I meant.
>>>>
>>> Is that really something we need to care about? If you link against
>>> 1.4.17
>>> and then, at runtime, use 1.3.4, could you expect that to work, then? I
>>> don't think we leave such guarantees.
>>>
>> Ah, yes, you're right. An application for 1.4.x libraries is not
>> expected to work when run with 1.3.x libraries, so we can add extra
>> fields in the next minor version, and don't care that an application
>> using them won't work if run with older libraries.
>>
>> The difference from our normal API compatibility process is that in this
>> case the incompatibility won't be detected at link time. If a
>> particular application uses some features of the new library, but only
>> ones of this kind, then it will load and start to run against old
>> libraries but with undefined behaviour. I don't know whether we want to
>> care about that.
>>
>
> Ouch! Even if this is permissible by a strict semantic reading of the
> compat rules, do we really want to create such a situation?
>
> Currently, the situation is that any compatibility problem will sooner
> or later trigger some kind of symbol-not-found error - a situation which
> allows for moderately graceful failure.
>
> I don't think it does either us, or the clients of our libraries, to
> avoid a little API revving if the cost is the potential for obscure and
> hard-to-detect bugs. I suggest simply contracting to rev the API
> whenever the structure changes (it's not that hard, after all).
>
We have version checks that enforce the compatibility rules.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [API] r18266
Posted by Max Bowsher <ma...@ukf.net>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Julian Foad wrote:
> Peter N. Lundblad wrote:
>> On Thu, 2 Feb 2006, Julian Foad wrote:
>>> Peter N. Lundblad wrote:
>>>
>>>> I don't understand the above. If we forbid users to allocate this
>>>> struct,
>>>> we can freely add fields in the future without problems. Or do you
>>>> mean
>>>> that we have a rule that if you compile with library x+1, and it links
>>>> with lbirary x, it should work? Is that what you mean by "forward
>>>> compatibility"?
>>>
>>> Yes, that's what I meant.
>>
>> Is that really something we need to care about? If you link against
>> 1.4.17
>> and then, at runtime, use 1.3.4, could you expect that to work, then? I
>> don't think we leave such guarantees.
>
> Ah, yes, you're right. An application for 1.4.x libraries is not
> expected to work when run with 1.3.x libraries, so we can add extra
> fields in the next minor version, and don't care that an application
> using them won't work if run with older libraries.
>
> The difference from our normal API compatibility process is that in this
> case the incompatibility won't be detected at link time. If a
> particular application uses some features of the new library, but only
> ones of this kind, then it will load and start to run against old
> libraries but with undefined behaviour. I don't know whether we want to
> care about that.
Ouch! Even if this is permissible by a strict semantic reading of the
compat rules, do we really want to create such a situation?
Currently, the situation is that any compatibility problem will sooner
or later trigger some kind of symbol-not-found error - a situation which
allows for moderately graceful failure.
I don't think it does either us, or the clients of our libraries, to
avoid a little API revving if the cost is the potential for obscure and
hard-to-detect bugs. I suggest simply contracting to rev the API
whenever the structure changes (it's not that hard, after all).
Max.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (Cygwin)
iD8DBQFD5Mv2fFNSmcDyxYARAt73AJ0d/lXpmiKCK25DUkCX4qZ9V83bRACeKpm8
RAxmUT1S+pFjpk+nX322gOQ=
=0xZx
-----END PGP SIGNATURE-----
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [API] r18266
Posted by Julian Foad <ju...@btopenworld.com>.
Peter N. Lundblad wrote:
> On Thu, 2 Feb 2006, Julian Foad wrote:
>>Peter N. Lundblad wrote:
>>
>>>I don't understand the above. If we forbid users to allocate this struct,
>>>we can freely add fields in the future without problems. Or do you mean
>>>that we have a rule that if you compile with library x+1, and it links
>>>with lbirary x, it should work? Is that what you mean by "forward
>>>compatibility"?
>>
>>Yes, that's what I meant.
>
> Is that really something we need to care about? If you link against 1.4.17
> and then, at runtime, use 1.3.4, could you expect that to work, then? I
> don't think we leave such guarantees.
Ah, yes, you're right. An application for 1.4.x libraries is not expected to
work when run with 1.3.x libraries, so we can add extra fields in the next
minor version, and don't care that an application using them won't work if run
with older libraries.
The difference from our normal API compatibility process is that in this case
the incompatibility won't be detected at link time. If a particular
application uses some features of the new library, but only ones of this kind,
then it will load and start to run against old libraries but with undefined
behaviour. I don't know whether we want to care about that.
I think the question is not specific to this patch, so I've committed it as
r18322 (with the caller updated too, of course - the patch I posted was
incomplete).
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [API] r18266
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Thu, 2 Feb 2006, Julian Foad wrote:
> Peter N. Lundblad wrote:
> > On Mon, 30 Jan 2006, Julian Foad wrote:
> >
> >> If we revise the
> >>function, we can't keep the same signature because although it will be
> >>compatible one way ("backward-compatible") it won't be the other way.
> [...]
> >
> > I don't understand the above. If we forbid users to allocate this struct,
> > we can freely add fields in the future without problems. Or do you mean
> > that we have a rule that if you compile with library x+1, and it links
> > with lbirary x, it should work? Is that what you mean by "forward
> > compatibility"?
>
> Yes, that's what I meant.
>
Is that really something we need to care about? If you link against 1.4.17
and then, at runtime, use 1.3.4, could you expect that to work, then? I
don't think we leave such guarantees.
Thanks,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [API] r18266
Posted by Julian Foad <ju...@btopenworld.com>.
Peter N. Lundblad wrote:
> On Mon, 30 Jan 2006, Julian Foad wrote:
>
>> If we revise the
>>function, we can't keep the same signature because although it will be
>>compatible one way ("backward-compatible") it won't be the other way.
[...]
>
> I don't understand the above. If we forbid users to allocate this struct,
> we can freely add fields in the future without problems. Or do you mean
> that we have a rule that if you compile with library x+1, and it links
> with lbirary x, it should work? Is that what you mean by "forward
> compatibility"?
Yes, that's what I meant.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [API] r18266
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Mon, 30 Jan 2006, Julian Foad wrote:
> Well, here's a patch, but...
>
> I'm not sure that the argument applies to this structure which is used
> only to return the result from this one function. If we revise the
> function, we can't keep the same signature because although it will be
> compatible one way ("backward-compatible") it won't be the other way.
> I'm not quite sure, but I think the benefit of an extensible structure
> only arises when it is used in multiple interfaces.
>
I don't understand the above. If we forbid users to allocate this struct,
we can freely add fields in the future without problems. Or do you mean
that we have a rule that if you compile with library x+1, and it links
with lbirary x, it should work? Is that what you mean by "forward
compatibility"?
Thanks,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [API] r18266
Posted by Julian Foad <ju...@btopenworld.com>.
Peter N. Lundblad wrote:
> Julian,
>
> In r18266, you added a new API svn_wc_revision_status with a pointer to
> struct svn_wc_revision_status_t, which will be filled by the function.
> This avoids lots of "out" parameters in the function, but it still makes
> it impossible to add new fields to the struct without revving both the
> struct and the function (which might be another reason for using a struct
> in the first place).
>
> I suggest making the function allocate the struct and adding to the struct
> doc a note that the size of this struct may increase in the future, so
> callers shouldn't create it themselves. (We can add constructors like
> we've done for other structs now or on demand later).
>
> We've made this mistake before leading to lots of API revision.
Well, here's a patch, but...
I'm not sure that the argument applies to this structure which is used only to
return the result from this one function. If we revise the function, we can't
keep the same signature because although it will be compatible one way
("backward-compatible") it won't be the other way. I'm not quite sure, but I
think the benefit of an extensible structure only arises when it is used in
multiple interfaces.
On the other hand, even if I'm right that this is not giving as much benefit as
multiply-used structures, it still seems better to do it than not. Yes?
For other structures, a "duplicate" function, a.k.a. "copy constructor", was,
as I recall, only needed by the swig/Python bindings or something. Can someone
say if one is likely to be needed here?
- Julian