Branko Čibej wrote: > On 19.11.2013 18:19, Julian Foad wrote: >> [...] 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?
Oops, yes I did mean that. >> 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'm a big fan of named constants in many cases but I compared the reading of the code in this case and '0' won out by being much more visually obvious as a terminator and not being in a different name-space (APR_SUCCESS to end a list of SVN_... codes looked odd and cluttered). And as it's a private API. >> 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? I r1536307 you did a lot, but only ones where the NULL had already been explicitly type-cast, which apparently wasn't close to all of them. Also I found another function to annotate since then (svn_ra_serf__add_open_tag_buckets), and probably some new instances crept in as well. It was some trouble for me to force the definition of NULL as plain 0 on my system, as at least one of the system header files forcibly redefines it to (void *)NULL even if I set it to plain NULL before including the headers. There was no single place I could set it. I still might have missed some. - Julian
