Re: Warning for missing sentinel arguments

Bert Huijben wrote:

>> Bert Huijben wrote:
>>> Julian Foad wrote:
>>>> On 11/18/13 3:03 PM, Julian Foad wrote:
>>>>> The patch also changes SVN_NO_ERROR from "0" to "((svn_error_t*)0)".
>>>>> This has the side effect of detecting other mis-uses: I committed two such
>>>>> fixes as http://svn.apache.org/r1543193 and http://svn.apache.org/r1543216
>>>>> . I can't think of any negative consequences but shout out if you can.
>>>
>>> Actually, this is a change of a public API and maybe ABI (I'm not sure), and
>>> while it might be a good idea in itself it should not be casually changed as
>>> part of this patch. So I'll leave out that change and not mark svn_cl__try()
>>> with SVN_SENTINEL_NULL, since GCC's attribute requires the sentinel argument
>>> to be a pointer.
>>
>> It is just compiler magic and doesn't affect the ABI or API.
[...]

I was referring to the change of SVN_NO_ERROR from '0' to '(void *)0'.
That's a change of public API. Third-party users will see that change.

(It's not an ABI change of course, because preprocessor macros aren't exposed in the ABI.)

If
 a third party is just using SVN_NO_ERROR in the intended way, this will be
 a good change. If they are using it in an unintended way (where some
non-svn_error_t zero value is wanted) then they will henceforth
potentially get a warning or an error or possibly even a change of
behaviour.

But now I see that the real issue with svn_cl__try is its variable arguments are not pointers at all, but error codes of (integer) type 'apr_status_t'. So the sentinel should be 0 or APR_SUCCESS.

I've changed the sentinels there to 0 and updated the doc string, in 1543507.

Earlier, in r1543477, I updated all remaining variadic function calls that want a pointer-null sentinel, to use SVN_VA_NULL instead of plain NULL.

- Julian
Received on 2013-11-19 18:20:21 CET