On Tue, 22 Mar 2005, Philip Martin wrote:
> Philip Martin <philip@codematters.co.uk> writes:
>
> > Ben Collins-Sussman <sussman@collab.net> writes:
> >
> > I see that you changed the svn_wc_entry_t struture:
> >
> > - I have a nasty feeling that it qualifies as an ABI change, so the
> > rules require a new svn_wc_entry2_t. Yuck! I really don't like the
> > size of the changes we would need to accomodate that.
> >
> > Perhaps the lock data could be stored in a separate cache within the
> > access baton, then no change to svn_wc_entry_t would be required.
> > A separate cache could avoid using any significant memory if there are
> > no locks.
>
OTOH, there are big comments both at the beginnin and end of the struct
definition talking about what should happen if one extends that struct. So
a user reading the header should probably be aware that this struct may be
extended. Or were these comments only for pre-1.0? Then they should be
removed IMO. I am not against caching this elsewhere; I am not sure it is
necessary, however. Also, be aware that the lock information is used in
the status callback. Currently it is fetched through the entry.
> I see that svn_wc_status_t and svn_client_ctx_t have also
been > changed, as well as svn_wc_entry_t noted above. In all cases new
> members have been added to the end of a structure, and I see that the
> new svn_wc_notify_t structure advises that doing this for ABI
> compatibility. While it is possible to write code that is compatible
> with such changes, it is also trivial to write code that will be
> broken. We have to consider third-party code as well as the
> Subversion code, these are public structures.
We have to be more precise when defining what we consider breaks the ABI.
Code can always depend on somethings size. One question is: do we consider
it ABI breakage to extend a struct when none of our functions takes it as
input from the caller? For example, would extending
svn_client_commit_item_t be valid? You may write a program that crashes,
but do we care? (This may apply to adding new values to enums and use
previously unused bit fields as well.) Yes, this is getting extreme. We
have to draw the line somewhere.
Now, back to the actual question at hand. Since we have svn_wc_status_dup,
you may actually be right about svn_wc_status. <sigh!> A user could do:
svn_wc_status_t stat;
svn_wc_status_t *newstat = svn_wc_status_dup (&stat, pool);
So, if we revise this struct and create svn_wc_status2_t, we really want
to document that users must not allocate them by hand - and possibly
provide an svn_wc_create_status, but I'm not sure who will use it.
> > The svn_client_ctx_t change might well be
classified as acceptable,
> since the documentation advises that svn_client_create_context is used
Yes. If you create them by hand, you get the crash you deserve for not
reading API docs.
> to create/initialise such structures. I see we already made such a
> change to svn_ra_plugin_t in 1.1 and I guess that we always intended
> to be able to do that even if we did not document it.
>
We don't support third-party RA plugins. (See above about the border for
ABI compatibility.)
> For the other structures, namely svn_wc_status_t and svn_wc_entry_t no
> such warning is given and third party software may well be using these
> in a way that is incompatible with the current ABI changes.
>
>
We really need to learn this once and for all. Never allow API users to
allocate structs by hand. I'll add an svn_lock_create!
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Mar 23 09:25:27 2005