Re: [PATCH] non-null declarations

[Eric Blake]

Bruno Haible <bruno <at> clisp.org> writes:

> Two points I'm not sure about:
>   - Should the macro be called _GL_ARG_NONNULL or GL_ARG_NONNULL?

_GL_ARG_NONNULL is in the implementation-reserved namespace, so it is less 
likely to collide, but it is also more to type.  I don't really care one way or 
another.

>   - Is it worth putting the macro definition (always the same 10 lines of 
code)
>     into a separate file, like done with link-warning.h?

I'd go one step further - I'd like to see it in config.h, as was done with 
_UNUSED_PARAMETER_ in the AH_VERBATIM block of gnulib-common.m4.  In other 
words, it is something so frequently used that it is nicer to guarantee that it 
is always defined (if you followed the instructions of including <config.h> 
first).

>  extern int scandir (const char *dir, struct dirent ***namelist,
>                   int (*filter) (const struct dirent *),
> -                 int (*cmp) (const struct dirent **, const struct dirent 
**));
> +                 int (*cmp) (const struct dirent **, const struct dirent **))
> +     _GL_ARG_NONNULL ((1, 2, 4));

Is there any desire to provide guaranteed extension semantics that cmp can be 
NULL in order to get an unsorted subset returned?

> -extern locale_t duplocale (locale_t locale);
> +extern locale_t duplocale (locale_t locale) _GL_ARG_NONNULL ((1));

Are we guaranteed that locale_t is a pointer type; or can it be a scalar on 
some platforms, in which case _GL_ARG_NONNULL might cause grief?

>  extern int fprintf (FILE *fp, const char *format, ...)
> -       __attribute__ ((__format__ (__printf__, 2, 3)));
> +       __attribute__ ((__format__ (__printf__, 2, 3))) _GL_ARG_NONNULL ((2));

Missed arg 1 (in two declarations of fprintf).

> -extern FILE * freopen (const char *filename, const char *mode, FILE *stream);
> +extern FILE * freopen (const char *filename, const char *mode, FILE *stream)
> +     _GL_ARG_NONNULL ((1, 2, 3));

Bug: Arg 1 should not be listed.  Cygwin DEPENDS on being able to pass NULL 
filename to change streams from text to binary.

>  extern int snprintf (char *str, size_t size, const char *format, ...)
> -       __attribute__ ((__format__ (__printf__, 3, 4)));
> +       __attribute__ ((__format__ (__printf__, 3, 4)))
> +       _GL_ARG_NONNULL ((1, 3));

Bug: Arg 1 should not be listed.  POSIX requires that str may be null if size 
is 0.

> -extern int setenv (const char *name, const char *value, int replace);
> +extern int setenv (const char *name, const char *value, int replace)
> +     _GL_ARG_NONNULL ((1));

POSIX and gnulib guarantee that calling this with NAME set to NULL will 
gracefully fail with EINVAL, so maybe we shouldn't list this one.  On the other 
hand, the POSIX folks are considering relaxing this (at my suggestion), so if 
we DO keep this nonnull decoration, then I need to do a corresponding 
relaxation of the testsuite to not pass NULL in the first place.
http://austingroupbugs.net/view.php?id=185

> -extern int unsetenv (const char *name);
> +extern int unsetenv (const char *name) _GL_ARG_NONNULL ((1));

Same as for setenv.

>  extern void *memchr (void const *__s, int __c, size_t __n)
> -  __attribute__ ((__pure__));
> +     __attribute__ ((__pure__)) _GL_ARG_NONNULL ((1));

This one I'm fuzzy on.  On one hand, the POSIX definition is that the function 
reads at most __c bytes, so when __c is 0, it is safe for __s to be NULL.  On 
the other hand, glibc already warns here, the gnulib testsuite already jumps 
through hoops to avoid the warning, and the likelihood of meaning to pass NULL 
when __c is 0 is slim.  Your call.

>  extern void *memmem (void const *__haystack, size_t __haystack_len,
>                    void const *__needle, size_t __needle_len)
> -  __attribute__ ((__pure__));
> +     __attribute__ ((__pure__)) _GL_ARG_NONNULL ((1, 3));

Same goes for all of the mem* interfaces, when length is 0.

> -extern int rpl_recvfrom (int, void *, int, int, struct sockaddr *, int *);
> +extern int rpl_recvfrom (int, void *, int, int, struct sockaddr *, int *)
> +     _GL_ARG_NONNULL ((2, 6));

Bug: Arg 6 can be NULL, since POSIX states that "The recv( ) function is 
equivalent to recvfrom( ) with a zero address_len argument" (well, technically, 
a null address_len argument only makes sense when address [arg 5] is also NULL).

> -extern int rpl_sendto (int, const void *, int, int, struct sockaddr *, int);
> +extern int rpl_sendto (int, const void *, int, int, struct sockaddr *, int)
> +     _GL_ARG_NONNULL ((2, 5));

Bug: Arg 5 can be NULL, since POSIX states that "The send( ) function is 
equivalent to sendto( ) with a null pointer dest_len argument" (well, that's 
also a bug in POSIX, since dest_len is socklen_t; it obviously meant a null 
dest_addr argument).

> -extern int futimens (int fd, struct timespec const times[2]);
> +extern int futimens (int fd, struct timespec const times[2])
> +     _GL_ARG_NONNULL ((2));

Bug: Arg 2 can be NULL, to specify 'now'.  This function should not have a 
nonnull decoration.

>     extern int utimensat (int fd, char const *name,
> -                         struct timespec const times[2], int flag);
> +                         struct timespec const times[2], int flag)
> +        _GL_ARG_NONNULL ((2, 3));

Likewise, Arg 3 should not be listed.

-- 
Eric Blake