On 9/1/2017 5:07 AM, Daniel Shahaf wrote:
> Evgeny Kotkov wrote on Thu, 31 Aug 2017 19:05 +0300:
>> Daniel Shahaf <d.s_at_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
Received on 2017-09-01 15:31:50 CEST