On 9/1/2017 5:07 AM, Daniel Shahaf wrote:
Evgeny Kotkov wrote on Thu, 31 Aug 2017 19:05 +0300:
Daniel Shahaf <d...@daniel.shahaf.name> writes:
Is adding this function an ABI-compatible change? The docstring of
svn_delta_editor_t does say """
* @note Don't try to allocate one of these yourself. Instead, always
* use svn_delta_default_editor() or some other constructor, to ensure
* that unused slots are filled in with no-op functions.
""", but an API consumer might have interpreted this note as meaning "You may
use malloc(..., sizeof(svn_delta_editor_t)) if you take care to initialize
all struct members", in which case, his code will not be ABI compatible
with 1.10.
I think that adding this callback does not affect the ABI compatibility.
The note says "Don't try to allocate one of these yourself", whereas the
malloc(..., sizeof(svn_delta_editor_t)) example does exactly the opposite.
I'm not sure I'm getting through here.
The note does say "Don't allocate one of these yourself" but in the
second sentence implies that the reason for this is that if you allocate
it yourself and don't initialize all pointer-to-function members,
Trouble will ensue. Therefore, the note could be construed as meaning
that it is okay to allocate one of these yourself if you take care to
initialize all pointer members.
In other words: the comment could have been interpreted as a piece
of advice --- "it is more robust to use _default_editor() than to allocate
with malloc" --- as opposed to a blanket prohibition on allocating this
struct type with malloc.
(If one uses malloc and doesn't initialize all pointers, the result
would be that an uninitialized pointer-to-function struct member is
dereferenced.)
In contrast, most of our other structs explicitly say that "Don't
allocate yourself because we may add new fields in the future". This
struct does not give that reason.
Makes sense?
I suppose can just add an API erratum and/or release notes entry about this.
Thanks,
Daniel
(I'll reply to your other points separately)
I think your proposal to add an erratum and "correct/update" the
documentation about allocating the struct is the right way to go here.
IMO this is only a minor inconsistency in the documentation (i.e. it
should be as clear as the rest of the API documentation with regards to
what the implications of not using the appropriate allocation methods are).
FWIW: I always read that current note already like this and interpreted
the sentence as just giving an example of why this is important and what
might go wrong if you don't follow the advice. But I certainly also see
that it might be interpreted in another way. So no harm in being precise
here and adding an erratum, I guess...
--
Regards,
Stefan Hett
