Re: [PATCH] compat/strcasestr: Include correct header file

[Tomi Ollila <tomi.ollila@iki.fi>]

On Sat, Dec 03 2022, Thomas Schneider wrote:

> Tomi Ollila <tomi.ollila@iki.fi> writes:
>
>> On Fri, Dec 02 2022, Thomas Schneider wrote:
>>
>>> As per strcasestr(3) of glibc and FreeBSD, the header that defines
>>> strcasestr() is string.h, not strings.h.  This may cause compilation,
>>> and thus detection whether an (optimised) version is available, to
>>> fail even if the function is available, when implicit declaration and
>>> pointer conversion do not match.
>>>
>>> Signed-off-by: Thomas Schneider <qsx@chaotikum.eu>
>>> ---
>>> I discovered this when building with Clang:
>>>
>>> qsx@naboo ~/src/notmuch (git)-[tags/0.32.2]
>>> % gcc -o /dev/null compat/have_strcasestr.c; echo $?
>>> compat/have_strcasestr.c: In function ‘main’:
>>> compat/have_strcasestr.c:10:13: warning: implicit declaration of function ‘strcasestr’; did you mean ‘strcasecmp’? [-Wimplicit-function-declaration]
>>>    10 |     found = strcasestr (haystack, needle);
>>>       |             ^~~~~~~~~~
>>>       |             strcasecmp
>>> compat/have_strcasestr.c:10:11: warning: assignment to ‘char *’ from ‘int’ makes pointer from integer without a cast [-Wint-conversion]
>>>    10 |     found = strcasestr (haystack, needle);
>>>       |           ^
>>> 0
>>> qsx@naboo ~/src/notmuch (git)-[tags/0.32.2]
>>> % clang -o /dev/null compat/have_strcasestr.c; echo $?
>>> compat/have_strcasestr.c:10:13: warning: call to undeclared function 'strcasestr'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
>>>     found = strcasestr (haystack, needle);
>>>             ^
>>> compat/have_strcasestr.c:10:11: error: incompatible integer to pointer conversion assigning to 'char *' from 'int' [-Wint-conversion]
>>>     found = strcasestr (haystack, needle);
>>>           ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> 1 warning and 1 error generated.
>>> 1
>>>
>>> configure then assumes strcasestr is not available when using Clang, so it
>>> builds the variant from compat/, which later fails when linking because of
>>> conflicting symbols.
>>>
>>> On a side note, debugging was more complicated that I’m used to, e. g.,
>>> autoconf’s config.log or similar output.
>>>
>>> ---
>>>  compat/have_strcasestr.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/compat/have_strcasestr.c b/compat/have_strcasestr.c
>>> index 3cd1838d..d52a62ec 100644
>>> --- a/compat/have_strcasestr.c
>>> +++ b/compat/have_strcasestr.c
>>> @@ -1,5 +1,5 @@
>>>  #define _GNU_SOURCE
>>> -#include <strings.h>
>>> +#include <string.h>
>>
>> Would it be better to include both strings.h and string.h, in
>> case some (rare) cases it worked only when strings.h were included
>> (or would that make the test fail in some cases then...?)
>
> I don’t think it would cause any issues, but neither would I assume both
> headers to be necessary somewhere.  Of course I haven’t tested it on Ye
> Olde Unix With Its Special Own Taste on a workstation that pales in
> comparison to my calculator, then again I assume they either (a) don’t
> have strcasestr() anyway, or (b) fail to support notmuch because of some
> other issues.
>
> 	--Thomas

So, string.h is "enough" to declare strcasecmp w/ glibc (-std=gnu*, but
not with -std=c99, std=c11 and so on !), any *bsd, probably macOS...
... but do we have *modern* systems where notmuch compiles but
strcasecmp() is not declared if strings.h is *not* defined ?

I'd write the above lines as 

#include <strings.h> /* strcasecmp() in POSIX */
#include <string.h> /* strcasecmp() in *BSD */

(or something, if for nothing else to tell the world notmuch developers 
knows about these things...)

Also, it would be nice is commit message were more accurate to
tell what is going on...

- strings.h is not "incorrect" header file for strcasecmp()
- glibc does not declare strcasecmp() in strings.h -- string.h includes
  strings.h when certain criteria is met (*), (**)


Related information:

(*) test compilation in linux (fedora 37):

$ printf %s\\n '#include <string.h>' \
        'int main(void) { return strcasecmp("a", "b"); }' \
        | gcc --std=c11 -xc -
<stdin>: In function ‘main’:
<stdin>:2:25: warning: implicit declaration of function ‘strcasecmp’; did
you mean ‘strncmp’? [-Wimplicit-function-declaration]

--

https://pubs.opengroup.org/onlinepubs/009696799/functions/strcasecmp.html

includes strings.h -- like linux strcasecmp(3) manual pages.

The linux strcasecmp(3) manual page notes (**):

  The strcasecmp() and strncasecmp() functions first appeared in
  4.4BSD, where they were declared in <string.h>.  Thus, for reasons
  of historical compatibility, the glibc <string.h> header file also
  declares these functions, if the _DEFAULT_SOURCE (or, in glibc 2.19
  and earlier, _BSD_SOURCE) feature test macro is defined.

>> Tomi

Thank you for your efforts to make notmuch better,
Tomi\r