Re: [PATCH] sig2str: Add tests.

[Collin Funk]

Hey Bruno,

On 3/13/24 2:57 PM, Bruno Haible wrote:
> Currently, these functions are only available in Solaris and IRIX
> (according to the 'show-portability' tool in
> gnulib/maint-tools/platforms/various-symlists [1]).

Thanks for the link. I don't have access to MacOS, HP-UX, or IRIX
machines so that is helpful. The test environment documentation also
looks useful. If there is any interest in GNU Hurd or Solaris then I
could write them once I end up needing a virtual machine for them
again. Speaking of Solaris, I believe that this line should have
"Solaris 11 OmniOS" instead of "Solaris 11 OmnioOS" [1].

> we should better wait for the new POSIX standard to become available,
> or for glibc to implement the two functions. (glibc, so far, has
> sigabbrev_np [3].)

That sounds good. I don't see sig2str used much so it is probably best
to wait until glibc or POSIX.

> The patch is nearly good. I'd like to ask for three things:

Sure. My first time touching files in modules/* and tests/* so I was
expecting it not to be perfect. :)

>   Rationale: This verifies, en passant, that the file can be #included
>   with only <config.h> as a prerequisite.

Ah, that makes sense. I am in the habit of doing #include <...> headers,
empty line, and then #include "..." headers. But since it makes the
tests less useful, your method works better.

I've changed it to this:

#include <config.h>

#include "sig2str.h"

#include <string.h>

#include "macros.h"

So we have sig2str.h included second and the minimal amount of headers
needed to compile. We get the definition of STREQ from macros.h but
must include <string.h> as well for strcmp.

> * In the .c file, separate individual tests somehow, for example through blank
>   lines:

Rationale makes sense to me. Fixed.

> * In the ChangeLog entry, list the "interesting" things first, and the
>   changes that are merely mechanical updates later. This makes it nicer
>   to follow the changes.

Fixed. Usually I just copied and pasted from git status. Your
reasoning makes sense though.

[1] 
https://git.savannah.gnu.org/cgit/gnulib/maint-tools.git/tree/platforms/test-environments.txt#n37

Collin

0001-sig2str-Add-tests.patch
Description: Text Data