Hello Collin,
> I noticed that sig2str and str2sig may be part of the next POSIX
> release and they are already provided by Solaris [1] [2].
>
> I think it would be nice to put these two functions inside of signal.h
> since it is becoming standardized.
Yes, when functions become standardized and are not available on all
platforms, they are good candidates for being supported by Gnulib.
Currently, these functions are only available in Solaris and IRIX
(according to the 'show-portability' tool in
gnulib/maint-tools/platforms/various-symlists [1]).
However, for the moment, a documentation how these functions shall
finally behave is not readily available. Currently all we have is [2].
I think for further actions (moving the declarations to <signal.h>)
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].)
> We can keep the sig2str.h header
> with a warning in a similar way to getprogname.h.
Yes. We avoid sudden incompatible changes. It is better to allow
a period of 1 or 2 years in which package maintainers can adapt
to changes made in Gnulib.
> Before doing that, I think it would be best to add a basic test to
> ensure no breakage occurs.
That's definititely a good idea, and can be done now already.
The patch is nearly good. I'd like to ask for three things:
* In the .c file, use as second #include statement the one that declares
the function(s). Currently this is sig2str.h, not <string.h> nor <signal.h>.
Rationale: This verifies, en passant, that the file can be #included
with only <config.h> as a prerequisite. In other words, it verifies that
the file is not referring to types or macros that would require other
#includes.
* In the .c file, separate individual tests somehow, for example through blank
lines:
/* Test sig2str on signals specified by ISO C. */
ASSERT (sig2str (SIGABRT, buffer) == 0);
ASSERT (STREQ (buffer, "ABRT"));
ASSERT (sig2str (SIGFPE, buffer) == 0);
ASSERT (STREQ (buffer, "FPE"));
ASSERT (sig2str (SIGILL, buffer) == 0);
ASSERT (STREQ (buffer, "ILL"));
ASSERT (sig2str (SIGINT, buffer) == 0);
ASSERT (STREQ (buffer, "INT"));
ASSERT (sig2str (SIGSEGV, buffer) == 0);
ASSERT (STREQ (buffer, "SEGV"));
ASSERT (sig2str (SIGTERM, buffer) == 0);
ASSERT (STREQ (buffer, "TERM"));
Rationale:
- It makes it easier to maintain the code.
- When a test fails and I want to run it under the debugger, often I want
to disable the earlier tests (that succeeded), so that my breakpoints
are only hit when it becomes interesting. The blank lines are ideal
places for adding #if 0 / #endif statements.
* 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. Especially since 'git format-patch' and 'git diff'
list the files in alphabetical order, it is useful to know "Where should
I start reading when I want to understand the patch?" In this case, the
.c file is the more interesting part, and the module description is a
consequence of the contents of the .c file.
Bruno
[1] https://git.savannah.gnu.org/gitweb/?p=gnulib/maint-tools.git;a=tree
[2] https://www.austingroupbugs.net/view.php?id=1138#c4975
[3] https://www.gnu.org/software/gnulib/manual/html_node/sigabbrev_005fnp.html
