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. br...@wandisco.com
