Re: Warning for missing sentinel arguments

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
Received on 2013-11-20 11:17:39 CET