On 19.11.2013 18:19, Julian Foad wrote:
> 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'.
I hope you mean (svn_error_t*)0, not (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.
Which is good -- the fact that SVN_NO_ERROR is not a pointer constant
can be considered an API bug, and it's OK to fix an API bug that does
not change the ABI. This change is essentially equivalent to a compiler
adding new warnings; existing, warning-free code may suddenly start
producing warnings or errors, but only if it is incorrect.
> 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.
Looks good; but I'd prefer that we consistently use APR_SUCCESS in this
case instead of 0. Sure it's the same thing, but using the named code
gives an extra hint that you're dealing with APR error codes.
> 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.
Hmmm ... I thought I'd covered all of them? Maybe new ones crept in
since then?
-- Brane
--
Branko Čibej | Director of Subversion
WANdisco // Non-Stop Data
e. brane_at_wandisco.com
Received on 2013-11-19 22:10:13 CET