Re: Gnulib and nullptr (and m4_provide_if)

[Akim Demaille]

Hi Friends,

> Le 9 févr. 2023 à 01:42, Paul Eggert  a écrit :
> 
> On 2/8/23 15:15, Bruno Haible wrote:
>> m4_provide_if is not documented in the Autoconf manual. Let's hope
>> it won't change incompatibly...
> 
> Yes, I hope so too. Gnulib is already using m4_provide_if in 
> gnulib/m4/largefile.m4 (in code stolen from Autoconf) so to some extent we're 
> exposed already, though of course this gnulib/m4/nullptr.m4 change increases 
> Gnulib's exposure.
> 
> One possibility is for Autoconf to document m4_provide_if and friends, since 
> they haven't changed in two decades. I'll cc this email to Akim to see 
> whether he has insight as to why the Autoconf manual doesn't document 
> m4_provide_if etc. already.

I have absolutely no idea :)  But it seems that m4_provide_if is considered to 
be part of m4sugar, and AC_PROVIDE_IFELSE to be its alter ego for Autoconf.  
Not documented either...

Cheers!

Re: Bison submit patches

[Akim Demaille]

Hi Bruno,

Thanks a lot for the analysis, and suggestion.  Thanks to Andrei for the 
report, and to Kaz and Paul for routing them to gnulib.

> Le 23 août 2022 à 22:53, Bruno Haible  a écrit :
> 
> The second patch is against Bison proper, and can be simplified like this:
> 
> diff --git a/src/location.c b/src/location.c
> index 5edce82c..86e7c39d 100644
> --- a/src/location.c
> +++ b/src/location.c
> @@ -268,7 +268,7 @@ caret_set_file (const char *file)
>   if (!caret_info.pos.file)
> {
>   caret_info.pos.file = file;
> -  if ((caret_info.file = fopen (caret_info.pos.file, "r")))
> +  if ((caret_info.file = fopen (caret_info.pos.file, "rb")))
> {
>   /* If the file is not regular (imagine #line 1 "/dev/stdin"
>  in the input file for instance), don't try to quote the
> 
> It rationale is that caret_getc_internal already handles the CR/LF
> newlines from Windows, and therefore opening the file in binary mode
> avoids the horrible kludges of the Microsoft stdio runtime for O_TEXT
> files.

I'm installing this:

commit cfef21f5b0a5c4291dcaa019e287210064371edb
Author: Akim Demaille 
Date:   Sat Sep 3 08:51:17 2022 +0200

diagnostics: Windows compatibility issues

Suggested by Bruno Haible
<https://lists.gnu.org/r/bug-bison/2022-08/msg6.html>
following a report from Andrei Malashkin
<https://lists.gnu.org/r/bug-bison/2022-08/msg3.html>

* src/location.c (caret_set_file): Read the file in binary.
We already deal with CRLF in caret_getc_internal.

diff --git a/THANKS b/THANKS
index 391b847e..52092368 100644
--- a/THANKS
+++ b/THANKS
@@ -14,6 +14,7 @@ Alexandre Duret-Lutz  a...@lrde.epita.fr
 Andre da Costa Barros andre.cbar...@yahoo.com
 Andreas Damm  ad...@onica.com
 Andreas Schwabsch...@suse.de
+Andrei Malashkin  malashkin.and...@gmail.com
 Andrew Suffield   asuffi...@users.sourceforge.net
 Angelo Borsotti   angelo.borso...@gmail.com
 Anthony Heading   a...@ajrh.net
diff --git a/src/location.c b/src/location.c
index 94c77ef6..0f56bd39 100644
--- a/src/location.c
+++ b/src/location.c
@@ -268,13 +268,13 @@ caret_set_file (const char *file)
   if (!caret_info.pos.file)
 {
   caret_info.pos.file = file;
-  if ((caret_info.file = fopen (caret_info.pos.file, "r")))
+  if ((caret_info.file = fopen (caret_info.pos.file, "rb")))
 {
   /* If the file is not regular (imagine #line 1 "/dev/stdin"
  in the input file for instance), don't try to quote the
- file.  Keep caret_info.file set so that we don't try to
- open it again, but leave caret_info.file NULL so that we
- don't try to quote it. */
+ file.  Keep caret_info.pos.file set so that we don't try
+ to open it again, but leave caret_info.file NULL so that
+ we don't try to quote it. */
   struct stat buf;
   if (fstat (fileno (caret_info.file), ) == 0
   && buf.st_mode & S_IFREG)

Re: Gnulib-related problems building Bison from git

[Akim Demaille]

Hi Bruno,

> Le 13 juil. 2022 à 08:41, Akim Demaille  a écrit :
> 
> What do you think about this?  Can I install it?

I pushed it.  Bison does not build successfully without libtextstyle without 
that.

I've installed the following patch in Bison.

Cheers!

commit c70b68c60d821f200bd3640b236437eae7085b76
Author: Akim Demaille 
Date:   Sun Jul 31 11:05:57 2022 +0200

gnulib: update

We now build cleanly even if libtextstyle is not available.
See https://lists.gnu.org/r/bug-bison/2022-07/msg2.html.

diff --git a/bootstrap.conf b/bootstrap.conf
index 6e9dd1cd..d33e9c8c 100644
--- a/bootstrap.conf
+++ b/bootstrap.conf
@@ -79,7 +79,7 @@ XGETTEXT_OPTIONS_RUNTIME=$XGETTEXT_OPTIONS'\\\
  --keyword=YY_ \\\
 '
 
-gnulib_tool_option_extras='--symlink --conditional-dependencies 
--makefile-name=gnulib.mk --automake-subdir --po-base=gnulib-po 
--po-domain=bison'
+gnulib_tool_option_extras='--symlink --conditional-dependencies 
--makefile-name=gnulib.mk --automake-subdir --automake-subdir-tests 
--po-base=gnulib-po --po-domain=bison'
 
 bootstrap_post_import_hook()
 {
diff --git a/gnulib b/gnulib
index 1803e3eb..76c7703c 16
--- a/gnulib
+++ b/gnulib
@@ -1 +1 @@
-Subproject commit 1803e3ebe5e956dda2d3483bc16b88f57f66a4df
+Subproject commit 76c7703cb2e9e0e803d1296618d8ab9e86e13d6c

Re: Gnulib-related problems building Bison from git

[Akim Demaille]

Hi Bruno,

> Le 6 juil. 2022 à 07:57, Akim Demaille  a écrit :
> 
> commit 813e5a1787ed156c70bd6d4bba39a8b2db4916db
> Author: Akim Demaille 
> Date:   Mon Jul 4 07:18:07 2022 +0200
> 
>gnulib-tool: add support for --automake-subdir-tests
> 
><https://lists.gnu.org/r/bug-gnulib/2022-01/msg00111.html>
> 
>* gnulib-tool (main): Handle --automake-subdir-tests.
>(func_emit_shellvars_init, func_emit_lib_Makefile_am): Use
>$sourcebase when handling tests and --automake-subdir-tests is
>given.
>(func_append_actionarg): Support --automake-subdir-tests.
>(func_create_testdir): Add missing argument for func_emit_initmacro_end.

What do you think about this?  Can I install it?

Re: Gnulib-related problems building Bison from git

[Akim Demaille]

> Le 6 juil. 2022 à 07:53, Akim Demaille  a écrit :
> 
> Here's my updated proposal.

I had forgotten to s/test/tests/ in the commit message, sorry.


commit 813e5a1787ed156c70bd6d4bba39a8b2db4916db
Author: Akim Demaille 
Date:   Mon Jul 4 07:18:07 2022 +0200

gnulib-tool: add support for --automake-subdir-tests

<https://lists.gnu.org/r/bug-gnulib/2022-01/msg00111.html>

* gnulib-tool (main): Handle --automake-subdir-tests.
(func_emit_shellvars_init, func_emit_lib_Makefile_am): Use
$sourcebase when handling tests and --automake-subdir-tests is
given.
(func_append_actionarg): Support --automake-subdir-tests.
(func_create_testdir): Add missing argument for func_emit_initmacro_end.

diff --git a/ChangeLog b/ChangeLog
index 8694db8900..fb7340e5ea 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,14 @@
+2022-07-06  Akim Demaille  
+
+   gnulib-tool: add support for --automake-subdir-tests
+   <https://lists.gnu.org/r/bug-gnulib/2022-01/msg00111.html>
+   * gnulib-tool (main): Handle --automake-subdir-tests.
+   (func_emit_shellvars_init, func_emit_lib_Makefile_am): Use
+   $sourcebase when handling tests and --automake-subdir-tests is
+   given.
+   (func_append_actionarg): Support --automake-subdir-tests.
+   (func_create_testdir): Add missing argument for func_emit_initmacro_end.
+
 2022-07-03  Bruno Haible  
 
lib-symbol-visibility: Improve documentation.
diff --git a/gnulib-tool b/gnulib-tool
index 5993143f3c..21aa6c4fab 100755
--- a/gnulib-tool
+++ b/gnulib-tool
@@ -311,6 +311,8 @@ Options for --import, --add/remove-import:
   --automake-subdir Specify that the makefile in the source-base
 directory be generated in such a way that it can
 be 'include'd from the toplevel Makefile.am.
+  --automake-subdir-tests
+Likewise, but for the tests directory.
   --macro-prefix=PREFIX  Specify the prefix of the macros 'gl_EARLY' and
 'gl_INIT'. Default is 'gl'.
   --po-domain=NAME  Specify the prefix of the i18n domain. Usually use
@@ -1118,7 +1120,8 @@ func_determine_path_separator
 # - gnu_maketrue if --gnu-make was given, false otherwise
 # - makefile_name   from --makefile-name
 # - tests_makefile_name  from --tests-makefile-name
-# - automake_subdir  true if --automake-subdir was given, false otherwise
+# - automake_subdirtrue if --automake-subdir was given, false otherwise
+# - automake_subdir_tests  true if --automake-subdir-tests was given, false 
otherwise
 # - libtool true if --libtool was given, false if --no-libtool was
 #   given, blank otherwise
 # - macro_prefixfrom --macro-prefix
@@ -1167,6 +1170,7 @@ func_determine_path_separator
   makefile_name=
   tests_makefile_name=
   automake_subdir=false
+  automake_subdir_tests=false
   libtool=
   macro_prefix=
   po_domain=
@@ -1414,6 +1418,9 @@ func_determine_path_separator
   --automake-subdir )
 automake_subdir=true
 shift ;;
+  --automake-subdir-tests )
+automake_subdir_tests=true
+shift ;;
   --libtool )
 libtool=true
 shift ;;
@@ -1527,6 +1534,7 @@ func_determine_path_separator
|| test -n "$excl_privileged_tests" || test -n "$excl_unportable_tests" 
\
|| test -n "$avoidlist" || test -n "$lgpl" || test -n "$makefile_name" \
|| test -n "$tests_makefile_name" || test "$automake_subdir" != false \
+   || test "$automake_subdir_tests" != false \
|| test -n "$macro_prefix" || test -n "$po_domain" \
|| test -n "$witness_c_macro" || test -n "$vc_files"; then
   echo "gnulib-tool: invalid options for 'update' mode" 1>&2
@@ -1617,8 +1625,8 @@ func_determine_path_separator
   func_fatal_error "minimum supported autoconf version is 2.64. Try adding 
AC_PREREQ([$DEFAULT_AUTOCONF_MINVERSION]) to your configure.ac." ;;
   esac
 
-  # Determine whether --automake-subdir is supported.
-  if $automake_subdir; then
+  # Determine whether --automake-subdir/--automake-subdir-tests are supported.
+  if $automake_subdir || $automake_subdir_tests; then
 found_subdir_objects=false
 if test -n "$configure_ac"; then
   my_sed_traces='
@@ -1644,7 +1652,7 @@ func_determine_path_separator
   done
 fi
 if ! $found_subdir_objects; then
-  func_fatal_error "Option --automake-subdir is only supported if the 
definition of AUTOMAKE_OPTIONS in Makefile.am contains 'subdir-objects'."
+  func_fatal_error "Option --automake-subdir/--automake-subdir-tests are 
only supported if the definition of AUTOMAKE_OPTIONS in Makefile.am contains 
'subdir-objects'."
 fi
   fi
 
@@ -4520,6

Re: Gnulib-related problems building Bison from git

[Akim Demaille]

Hi Bruno,

Thanks for the feedback!

> Le 4 juil. 2022 à 23:31, Bruno Haible  a écrit :
> 
>> @@ -4518,7 +4526,9 @@ func_emit_initmacro_end ()
>>   echo "  sed_dirname4='s,\\(.\\)/[^/]*\$,\\1,'"
>>   echo "  sed_basename1='s,.*/,,'"
>>   echo "changequote([, ])dnl"
>> -  if $automake_subdir && ! "$2" && test -n "$sourcebase" && test 
>> "$sourcebase" != '.'; then
>> +  if (($2 && $automake_subdir_test) || (! $2 && $automake_subdir)) \
>> + && test -n "$sourcebase" \
>> + && test "$sourcebase" != '.'; then
>> subdir="$sourcebase/"
>>   else
>> subdir=
> 
> Three things are wrong here:
>  - If $2, subdir should be set to "$testsbase/", not "$sourcebase/".

That was unclear to me, I expected to put everything in lib/, but
it does work with testsbase, thanks!

>  - Parentheses introduce a subshell, which is more expensive to execute
>than a compound shell command. IOW,
>  { command1 && command2; } || { command3 && command4; }
>is more efficient than
>  (command1 && command2) || (command3 && command4)

Sure...  I'm getting rusty.

>  - Some bash versions interpret '((' as the start of an evaluation
>command. To please these shells, you would have to add a space,
>i.e. write   ( (   instead of   ((

I was unaware of this.  Actually I should not have used (...), so
I shouldn't have needed to know :)

> I don't believe that the unconditional reference to $sourcebase is
> correct here. Look at the various invocations of func_emit_shellvars_init;
> you need to accommodate all different cases.

You are right, I had overlooked other calls.

I do not understand all the details.  Here's my updated proposal.

Cheers.

commit 18d46e1a0ef90bd8425db984d8cf811e7d690394
Author: Akim Demaille 
Date:   Mon Jul 4 07:18:07 2022 +0200

gnulib-tool: add support for --automake-subdir-tests

<https://lists.gnu.org/r/bug-gnulib/2022-01/msg00111.html>

* gnulib-tool (main): Handle --automake-subdir-test.
(func_emit_shellvars_init, func_emit_lib_Makefile_am): Use
$sourcebase when handling tests and --automake-subdir-test is
given.
(func_append_actionarg): Support --automake-subdir-test.
(func_create_testdir): Add missing argument for func_emit_initmacro_end.

diff --git a/ChangeLog b/ChangeLog
index 8694db8900..1130ffd6cb 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,14 @@
+2022-07-06  Akim Demaille  
+
+   gnulib-tool: add support for --automake-subdir-test
+   <https://lists.gnu.org/r/bug-gnulib/2022-01/msg00111.html>
+   * gnulib-tool (main): Handle --automake-subdir-test.
+   (func_emit_shellvars_init, func_emit_lib_Makefile_am): Use
+   $sourcebase when handling tests and --automake-subdir-test is
+   given.
+   (func_append_actionarg): Support --automake-subdir-test.
+   (func_create_testdir): Add missing argument for func_emit_initmacro_end.
+
 2022-07-03  Bruno Haible  
 
lib-symbol-visibility: Improve documentation.
diff --git a/gnulib-tool b/gnulib-tool
index 5993143f3c..21aa6c4fab 100755
--- a/gnulib-tool
+++ b/gnulib-tool
@@ -311,6 +311,8 @@ Options for --import, --add/remove-import:
   --automake-subdir Specify that the makefile in the source-base
 directory be generated in such a way that it can
 be 'include'd from the toplevel Makefile.am.
+  --automake-subdir-tests
+Likewise, but for the tests directory.
   --macro-prefix=PREFIX  Specify the prefix of the macros 'gl_EARLY' and
 'gl_INIT'. Default is 'gl'.
   --po-domain=NAME  Specify the prefix of the i18n domain. Usually use
@@ -1118,7 +1120,8 @@ func_determine_path_separator
 # - gnu_maketrue if --gnu-make was given, false otherwise
 # - makefile_name   from --makefile-name
 # - tests_makefile_name  from --tests-makefile-name
-# - automake_subdir  true if --automake-subdir was given, false otherwise
+# - automake_subdirtrue if --automake-subdir was given, false otherwise
+# - automake_subdir_tests  true if --automake-subdir-tests was given, false 
otherwise
 # - libtool true if --libtool was given, false if --no-libtool was
 #   given, blank otherwise
 # - macro_prefixfrom --macro-prefix
@@ -1167,6 +1170,7 @@ func_determine_path_separator
   makefile_name=
   tests_makefile_name=
   automake_subdir=false
+  automake_subdir_tests=false
   libtool=
   macro_prefix=
   po_domain=
@@ -1414,6 +1418,9 @@ func_determine_path_separator
   --automake-subdir )
 automake_subdir=true
 shift ;;
+  --automake-subdir-tes

Re: Gnulib-related problems building Bison from git

[Akim Demaille]

Hi,

Since Bison was moved to use gnulib's support for --automake-subdir, the 
configuration fails for people who don't have libtextstyle installed.  Or 
distcheck fails when libtextstyle is not installed in a default include path.

This is because --automake-subdir is built (apparently) for projects that use a 
single Makefile for the sources, but another for the tests.  This is not the 
case of Bison, where we have a single Makefile for the whole project.

In <https://lists.gnu.org/r/bug-gnulib/2022-01/msg00156.html> I had asked what 
was the preferred fix.  Below, I chose to add another option, 
--automake-subdir-test to signal cases such as Bison's.

If this is validated, I'll complete it with the doc parts.

Cheers!

commit b781a18296d4d5df0325f6618d296b8bac6e6b17
Author: Akim Demaille 
Date:   Mon Jul 4 07:18:07 2022 +0200

gnulib-tool: add support for --automake-subdir-test

<https://lists.gnu.org/r/bug-gnulib/2022-01/msg00111.html>
* gnulib-tool (main): Handle --automake-subdir-test.
(func_emit_shellvars_init, func_emit_lib_Makefile_am): Use
$sourcebase when handling tests and --automake-subdir-test is
given.
(func_append_actionarg): Support --automake-subdir-test.

diff --git a/ChangeLog b/ChangeLog
index 2daa6d8c81..ddb8c8523e 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,13 @@
+2022-07-04  Akim Demaille  
+
+   gnulib-tool: add support for --automake-subdir-test
+   <https://lists.gnu.org/r/bug-gnulib/2022-01/msg00111.html>
+   * gnulib-tool (main): Handle --automake-subdir-test.
+   (func_emit_shellvars_init, func_emit_lib_Makefile_am): Use
+   $sourcebase when handling tests and --automake-subdir-test is
+   given.
+   (func_append_actionarg): Support --automake-subdir-test.
+
 2022-06-12  Paul Eggert  
 
fchmodat: port better to MS-Windows etc.
diff --git a/gnulib-tool b/gnulib-tool
index 5993143f3c..0ba6951cd7 100755
--- a/gnulib-tool
+++ b/gnulib-tool
@@ -311,6 +311,8 @@ Options for --import, --add/remove-import:
   --automake-subdir Specify that the makefile in the source-base
 directory be generated in such a way that it can
 be 'include'd from the toplevel Makefile.am.
+  --automake-subdir-test
+Likewise, but for the test directory.
   --macro-prefix=PREFIX  Specify the prefix of the macros 'gl_EARLY' and
 'gl_INIT'. Default is 'gl'.
   --po-domain=NAME  Specify the prefix of the i18n domain. Usually use
@@ -1118,7 +1120,8 @@ func_determine_path_separator
 # - gnu_maketrue if --gnu-make was given, false otherwise
 # - makefile_name   from --makefile-name
 # - tests_makefile_name  from --tests-makefile-name
-# - automake_subdir  true if --automake-subdir was given, false otherwise
+# - automake_subdir   true if --automake-subdir was given, false otherwise
+# - automake_subdir_test  true if --automake-subdir-test was given, false 
otherwise
 # - libtool true if --libtool was given, false if --no-libtool was
 #   given, blank otherwise
 # - macro_prefixfrom --macro-prefix
@@ -1167,6 +1170,7 @@ func_determine_path_separator
   makefile_name=
   tests_makefile_name=
   automake_subdir=false
+  automake_subdir_test=false
   libtool=
   macro_prefix=
   po_domain=
@@ -1414,6 +1418,9 @@ func_determine_path_separator
   --automake-subdir )
 automake_subdir=true
 shift ;;
+  --automake-subdir-test )
+automake_subdir_test=true
+shift ;;
   --libtool )
 libtool=true
 shift ;;
@@ -1527,6 +1534,7 @@ func_determine_path_separator
|| test -n "$excl_privileged_tests" || test -n "$excl_unportable_tests" 
\
|| test -n "$avoidlist" || test -n "$lgpl" || test -n "$makefile_name" \
|| test -n "$tests_makefile_name" || test "$automake_subdir" != false \
+   || test "$automake_subdir_test" != false \
|| test -n "$macro_prefix" || test -n "$po_domain" \
|| test -n "$witness_c_macro" || test -n "$vc_files"; then
   echo "gnulib-tool: invalid options for 'update' mode" 1>&2
@@ -1617,8 +1625,8 @@ func_determine_path_separator
   func_fatal_error "minimum supported autoconf version is 2.64. Try adding 
AC_PREREQ([$DEFAULT_AUTOCONF_MINVERSION]) to your configure.ac." ;;
   esac
 
-  # Determine whether --automake-subdir is supported.
-  if $automake_subdir; then
+  # Determine whether --automake-subdir/--automake-subdir-test are supported.
+  if $automake_subdir || $automake_subdir_test; then
 found_subdir_objects=false
 if test -n "$configure_ac"; then
   my_sed_traces='
@@ -1644,7 +1652,7 @@ func_determine_path_separator
   done
 fi
 if ! $found_subdir_objects; the

Re: Gnulib-related problems building Bison from git

[Akim Demaille]

Hi Paul,

> Le 15 janv. 2022 à 20:51, Paul Eggert  a écrit :
> 
> I can't build Bison from Savannah git. See attached transcript, which ends:
> 
>  GEN  lib/inttypes.h
> make: *** No rule to make target 'textstyle.h', needed by 'all'.  Stop.
> 
> The Makefile (also attached) has a rule to build lib/textstyle.h. However, it 
> says "TEXTSTYLE_H = textstyle.h" and "BUILT_SOURCES = ... $(TEXTSTYLE_H) ..." 
> and since $(BUILT_SOURCES) is a prerequisite for "all" the build fails.
> 
> I am building on Ubuntu 21.10, except with bleeding edge Autoconf 
> (2.72a.22-6027-dirty) because Bison requires at least Autoconf 2.71.

I can reproduce this, once I uninstalled libtextstyle.

The problem is my call to

> # We want ostream_printf and hyperlink support.
> gl_LIBTEXTSTYLE_OPTIONAL([0.20.5])

in configure.ac.  First, it was too early (right after gl_EARLY) and it 
generated

> TEXTSTYLE_H="${gl_source_base_prefix}textstyle.h"

before the definition of gl_source_base_prefix.


If I move it after gl_INIT, I get:

> $ grep gl_source_base_prefix configure
>   gl_source_base_prefix='$(top_build_prefix)lib/'
> ALLOCA_H="${gl_source_base_prefix}alloca.h"
> ERRNO_H="${gl_source_base_prefix}errno.h"
> FLOAT_H="${gl_source_base_prefix}float.h"
> GETOPT_H="${gl_source_base_prefix}getopt.h"
> GETOPT_CDEFS_H="${gl_source_base_prefix}getopt-cdefs.h"
> ICONV_H="${gl_source_base_prefix}iconv.h"
> ICONV_H="${gl_source_base_prefix}iconv.h"
> TEXTSTYLE_H="${gl_source_base_prefix}textstyle.h"
> LIMITS_H="${gl_source_base_prefix}limits.h"
> STDALIGN_H="${gl_source_base_prefix}stdalign.h"
> STDBOOL_H="${gl_source_base_prefix}stdbool.h"
> STDDEF_H="${gl_source_base_prefix}stddef.h"
> STDINT_H="${gl_source_base_prefix}stdint.h"
> LIMITS_H="${gl_source_base_prefix}limits.h"
>   LIBUNISTRING_UNISTR_H="${gl_source_base_prefix}unistr.h"
>   LIBUNISTRING_UNITYPES_H="${gl_source_base_prefix}unitypes.h"
>   LIBUNISTRING_UNIWIDTH_H="${gl_source_base_prefix}uniwidth.h"
>   gl_source_base_prefix=
> TEXTSTYLE_H="${gl_source_base_prefix}textstyle.h"

Both assignments to gl_source_base_prefix are baked in gl_INIT.  This second 
assignment is meant for the tests, it is from

>   gl_COMMON
>   gl_source_base='tests'
>   gl_source_base_prefix=

This is probably affecting all the modules for which the user is expected to 
call the macro by hand: the variables will not be set as expected.

I don't know what is the preferred fix here.  Bruno?

Re: bison segv under Cygwin 64 at fatal-signal.c:318

[Akim Demaille]

Hi Brian,

> Le 18 sept. 2021 à 19:04, Brian Inglis  a 
> écrit :
> 
> Thanks very much for your help Bruno, in diagnosing the issue correctly, and 
> providing the patch: I will ensure your patch comment gets prefixed into the 
> respun bison gnulib patch.
> 
> And thanks Akim for getting Bruno involved to address the right area.
> 
> Cygwin bison users will be happy having the latest build available.

I did not see the confirmation that the last tarball I made did work on your 
system.

https://www.lrde.epita.fr/~akim/private/bison/bison-3.8.1.29-5c106.tar.gz
https://www.lrde.epita.fr/~akim/private/bison/bison-3.8.1.29-5c106.tar.lz
https://www.lrde.epita.fr/~akim/private/bison/bison-3.8.1.29-5c106.tar.xz

I might have missed it though.

Thanks Bruno, thanks Brian.

Re: bison segv under Cygwin 64 at fatal-signal.c:318

[Akim Demaille]

> Le 18 sept. 2021 à 15:22, Brian Inglis  a 
> écrit :
> 
> Patch would not apply as included gnulib and bison threadlib.m4 appear to be 
> from July and August.

FWIW, the last tarball I provided you with had a very recent gnulib.  The 
tarball below has

commit 7818455627c5e54813ac89924b8b67d0bc869146 (HEAD -> master, origin/master, 
origin/HEAD)
Author: Bruno Haible 
Date:   Fri Sep 17 22:22:50 2021 +0200

threadlib: Avoid crashes in thread-related functions on Cygwin 3.2.0.

Reported by Brian Inglis via Akim Demaille in
<https://lists.gnu.org/archive/html/bug-gnulib/2021-09/msg00063.html>.

* m4/threadlib.m4 (gl_WEAK_SYMBOLS): Force a "guessing no" result on
Cygwin.

at its top.

https://www.lrde.epita.fr/~akim/private/bison/bison-3.8.1.29-5c106.tar.gz
https://www.lrde.epita.fr/~akim/private/bison/bison-3.8.1.29-5c106.tar.lz
https://www.lrde.epita.fr/~akim/private/bison/bison-3.8.1.29-5c106.tar.xz


Cheers!

Re: Portability issues of inline on macOS with GCC11

[Akim Demaille]

> Le 13 sept. 2021 à 07:30, Bruno Haible  a écrit :
> 
> Akim Demaille wrote:
>> The preprocessed source is
>> 
>>> static [[__maybe_unused__]] void *xnrealloc (void *p, size_t n, size_t s)
>>>  __attribute__ ((__alloc_size__ (2, 3)));
>>> static [[__maybe_unused__]] void *
>>> xnrealloc (void *p, size_t n, size_t s)
>>> {
>>>  return xreallocarray (p, n, s);
>>> }
> 
> Thanks for the report. This patch should fix it.

It does, thanks a lot!

Portability issues of inline on macOS with GCC11

[Akim Demaille]

Hi,

If I try to update gnulib in Bison, I have the following errors using GCC 
11.2.0 on macOS.

> In file included from /Users/akim/src/gnu/bison/src/system.h:74,
>  from /Users/akim/src/gnu/bison/src/Sbitset.c:21:
> /Users/akim/src/gnu/bison/lib/xalloc.h:139:1: error: 'maybe_unused' attribute 
> ignored [-Werror=attributes]
>   139 | XALLOC_INLINE void *xnrealloc (void *p, size_t n, size_t s)
>   | ^
> /Users/akim/src/gnu/bison/lib/xalloc.h:139:15: error: expected identifier or 
> '(' before 'void'
>   139 | XALLOC_INLINE void *xnrealloc (void *p, size_t n, size_t s)
>   |   ^~~~
> /Users/akim/src/gnu/bison/lib/xalloc.h:141:1: error: 'maybe_unused' attribute 
> ignored [-Werror=attributes]
>   141 | XALLOC_INLINE void *
>   | ^
> /Users/akim/src/gnu/bison/lib/xalloc.h:141:15: error: expected identifier or 
> '(' before 'void'
>   141 | XALLOC_INLINE void *
>   |   ^~~~
> cc1: all warnings being treated as errors
> gmake[2]: *** [Makefile:8758: src/bison-Sbitset.o] Error 1

The preprocessed source is

> static [[__maybe_unused__]] void *xnrealloc (void *p, size_t n, size_t s)
>   __attribute__ ((__alloc_size__ (2, 3)));
> static [[__maybe_unused__]] void *
> xnrealloc (void *p, size_t n, size_t s)
> {
>   return xreallocarray (p, n, s);
> }

As a matter of fact, there are many similar failures.

> In file included from /Users/akim/src/gnu/bison/lib/gl_map.c:21:
> /Users/akim/src/gnu/bison/lib/gl_map.h:263:1: warning: 'maybe_unused' 
> attribute ignored [-Wattributes]
>   263 | gl_map_t
>   | ^~~~
> /Users/akim/src/gnu/bison/lib/gl_map.h:263:1: error: expected identifier or 
> '(' before 'gl_map_t'

The preprocessor gives:

static [[__maybe_unused__]]

gl_map_t
gl_map_nx_create_empty (gl_map_implementation_t implementation,
gl_mapkey_equals_fn equals_fn,
gl_mapkey_hashcode_fn hashcode_fn,
gl_mapkey_dispose_fn kdispose_fn,
gl_mapvalue_dispose_fn vdispose_fn)
{
  return implementation->nx_create_empty (implementation,
  equals_fn, hashcode_fn,
  kdispose_fn, vdispose_fn);
}

The command is:

> ccache gcc-mp-11 -DEXEEXT=\"\"   -I. -I./lib -I/Users/akim/src/gnu/bison 
> -I/Users/akim/src/gnu/bison/lib -DINSTALLDIR=\"/opt/gostai/bin\" -isystem 
> /opt/gostai/include -isystem /opt/local/include -I/opt/gostai/include 
> -I/opt/local/include -Wunreachable-code -Wall -Wextra -Wcast-align 
> -Wchar-subscripts -Wformat -Wimplicit-fallthrough -Wmismatched-dealloc 
> -Wnull-dereference -Wno-sign-compare -Wpointer-arith -Wshadow 
> -Wstrict-aliasing -Wwrite-strings -Wbad-function-cast -Wmissing-prototypes 
> -Wstrict-prototypes  -Werror -ggdb -MT src/bison-AnnotationList.o -MD -MP -MF 
> src/.deps/bison-AnnotationList.Tpo -c -o src/bison-AnnotationList.o `test -f 
> 'src/AnnotationList.c' || echo 
> '/Users/akim/src/gnu/bison/'`src/AnnotationList.c
> In file included from /Users/akim/src/gnu/bison/lib/bitset/base.h:32,
>  from /Users/akim/src/gnu/bison/lib/bitset.h:31,
>  from /Users/akim/src/gnu/bison/lib/bitsetv.h:24,
>  from /Users/akim/src/gnu/bison/src/AnnotationList.h:23,
>  from /Users/akim/src/gnu/bison/src/AnnotationList.c:22:
> /Users/akim/src/gnu/bison/lib/xalloc.h:139:1: error: 'maybe_unused' attribute 
> ignored [-Werror=attributes]
>   139 | XALLOC_INLINE void *xnrealloc (void *p, size_t n, size_t s)
>   | ^
> /Users/akim/src/gnu/bison/lib/xalloc.h:139:15: error: expected identifier or 
> '(' before 'void'
>   139 | XALLOC_INLINE void *xnrealloc (void *p, size_t n, size_t s)
>   |   ^~~~
> /Users/akim/src/gnu/bison/lib/xalloc.h:141:1: error: 'maybe_unused' attribute 
> ignored [-Werror=attributes]
>   141 | XALLOC_INLINE void *
>   | ^
> /Users/akim/src/gnu/bison/lib/xalloc.h:141:15: error: expected identifier or 
> '(' before 'void'
>   141 | XALLOC_INLINE void *
>   |   ^~~~
> cc1: all warnings being treated as errors


I'm unsure how you would like to address this.

Cheers!


> /Users/akim/src/gnu/bison/lib/gl_map.h:275:1: warning: 'maybe_unused' 
> attribute ignored [-Wattributes]
>   275 | GL_MAP_INLINE size_t
>   | ^
> /Users/akim/src/gnu/bison/lib/gl_map.h:275:15: error: expected identifier or 
> '(' before 'size_t'
>   275 | GL_MAP_INLINE size_t
>   |   ^~
> /Users/akim/src/gnu/bison/lib/gl_map.h:281:1: warning: 'maybe_unused' 
> attribute ignored [-Wattributes]
>   281 | GL_MAP_INLINE bool
>   | ^
> In file included from /Users/akim/src/gnu/bison/lib/gl_map.h:21,
>  from /Users/akim/src/gnu/bison/lib/gl_map.c:21:
> /Users/akim/src/gnu/bison/lib/gl_map.h:281:15: error: expected identifier or 
> '(' before '_Bool'
>   281 | GL_MAP_INLINE bool
>   

snippet/_Noreturn: Fix typo

[Akim Demaille]

From a report by Christopher Nielsen  about Bison (which 
uses an almost verbatim copy of this snippet).

<https://trac.macports.org/ticket/59927#comment:59>
<https://trac.macports.org/ticket/59927#comment:62>

Cheers!

commit 964ce0a92b9ba87afe7787dc0fd5d1e1abe7214c
Author: Akim Demaille 
Date:   Thu Aug 12 09:30:41 2021 +0200

snippet/_Noreturn: Fix typo

* lib/_Noreturn.h: Fix spelling of 4.

diff --git a/ChangeLog b/ChangeLog
index 037fa7da4..b8a93468d 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2021-08-12  Akim Demaille  
+
+   snippet/_Noreturn: Fix typo
+   * lib/_Noreturn.h: Fix spelling of 4.
+
 2021-08-11  Paul Eggert  
 
dynarray: merge from glibc
diff --git a/lib/_Noreturn.h b/lib/_Noreturn.h
index cb72f2620..6fed3c797 100644
--- a/lib/_Noreturn.h
+++ b/lib/_Noreturn.h
@@ -29,7 +29,7 @@
 # elif ((!defined __cplusplus || defined __clang__) \
 && (201112 <= (defined __STDC_VERSION__ ? __STDC_VERSION__ : 0) \
 || (!defined __STRICT_ANSI__ \
-&& (__4 < __GNUC__ + (7 <= __GNUC_MINOR__) \
+&& (4 < __GNUC__ + (7 <= __GNUC_MINOR__) \
 || (defined __apple_build_version__ \
 ? 600 <= __apple_build_version__ \
 : 3 < __clang_major__ + (5 <= __clang_minor__))

Re: Portability issues with scractch_buffer

[Akim Demaille]

Hi Paul,

> Le 20 janv. 2021 à 21:14, Akim Demaille  a écrit :
> 
> Hi Paul,
> 
>> Le 20 janv. 2021 à 02:50, Paul Eggert  a écrit :
>> 
>> On 1/19/21 10:22 AM, Akim Demaille wrote:
>> 
>>> Clang 3.3 and 3.4 cannot compile the new scratch-buffer module.  On Bison's 
>>> CI (Clang 3.4: https://api.travis-ci.org/v3/job/755133481/log.txt), there 
>>> is:
>>>> ...
>>>>  CC   lib/lib_libbison_a-canonicalize.o
>>>> In file included from ../lib/canonicalize.c:31:
>>>> In file included from ../lib/scratch_buffer.h:28:
>>>> ../lib/malloc/scratch_buffer.h:69:11: error: unknown type name 
>>>> 'max_align_t'
>> 
>> Evidently the stddef module is not arranging for the replacement stddef.h to 
>> define max_align_t properly. Could you investigate why that might be? What 
>> does lib/stddef.h look like? What is the output of 'clang -E 
>> lib/lib_libbison_a-canonicalize.c' (with the proper compile-time options)?
> 
> See below for the output:
> 
> - clang 3.4: https://api.travis-ci.org/v3/job/755390946/log.txt
> - clang 3.3: https://api.travis-ci.org/v3/job/755390947/log.txt
> 
> Look for "make spy" in there.
> 
> Both come from this job:
> 
> - https://travis-ci.org/github/akimd/bison/builds/755390943

The problem is solved, the latest builds succeed on these platforms
(https://travis-ci.org/github/akimd/bison/jobs/766644360 and
https://travis-ci.org/github/akimd/bison/jobs/766644361).

Cheers!

fprintf-posix: fix typo

[Akim Demaille]

Installed.  Cheers!

commit e639e557f1b09a4f5589377705a30c4014b76924
Author: Akim Demaille 
Date:   Fri Mar 26 06:40:06 2021 +0100

fprintf-posix: fix typo

* modules/fprintf-posix (Depends-on): Fix typo.

diff --git a/ChangeLog b/ChangeLog
index b9fb4b797..cac41b795 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2021-03-26  Akim Demaille  
+
+   fprintf-posix: fix typo
+   * modules/fprintf-posix (Depends-on): Fix typo.
+
 2021-03-25  Paul Eggert  
 
free-posix: use more often in other modules
diff --git a/modules/fprintf-posix b/modules/fprintf-posix
index 123fbea2d..f2bd1a5c7 100644
--- a/modules/fprintf-posix
+++ b/modules/fprintf-posix
@@ -23,7 +23,7 @@ fseterr [test $REPLACE_FPRINTF = 1]
 vasnprintf  [test $REPLACE_FPRINTF = 1]
 isnand-nolibm   [test $REPLACE_FPRINTF = 1]
 isnanl-nolibm   [test $REPLACE_FPRINTF = 1]
-freee-posix [test $REPLACE_FPRINTF = 1]
+free-posix  [test $REPLACE_FPRINTF = 1]
 frexp-nolibm[test $REPLACE_FPRINTF = 1]
 frexpl-nolibm   [test $REPLACE_FPRINTF = 1]
 printf-frexp[test $REPLACE_FPRINTF = 1]

Re: Portability issues with scractch_buffer

[Akim Demaille]

Hi Paul,

> Le 20 janv. 2021 à 02:50, Paul Eggert  a écrit :
> 
> On 1/19/21 10:22 AM, Akim Demaille wrote:
> 
>> Clang 3.3 and 3.4 cannot compile the new scratch-buffer module.  On Bison's 
>> CI (Clang 3.4: https://api.travis-ci.org/v3/job/755133481/log.txt), there is:
>>> ...
>>>   CC   lib/lib_libbison_a-canonicalize.o
>>> In file included from ../lib/canonicalize.c:31:
>>> In file included from ../lib/scratch_buffer.h:28:
>>> ../lib/malloc/scratch_buffer.h:69:11: error: unknown type name 'max_align_t'
> 
> Evidently the stddef module is not arranging for the replacement stddef.h to 
> define max_align_t properly. Could you investigate why that might be? What 
> does lib/stddef.h look like? What is the output of 'clang -E 
> lib/lib_libbison_a-canonicalize.c' (with the proper compile-time options)?

See below for the output:

- clang 3.4: https://api.travis-ci.org/v3/job/755390946/log.txt
- clang 3.3: https://api.travis-ci.org/v3/job/755390947/log.txt

Look for "make spy" in there.

Both come from this job:

- https://travis-ci.org/github/akimd/bison/builds/755390943

>>> ../lib/canonicalize.c:237:15: warning: declaration does not declare 
>>> anything [-Wmissing-declarations]
>>>   FALLTHROUGH;
>>>   ^~~
>>> ../lib/attribute.h:142:21: note: expanded from macro 'FALLTHROUGH'
>>> #define FALLTHROUGH _GL_ATTRIBUTE_FALLTHROUGH
>>> ^
>>> ./lib/config.h:2016:36: note: expanded from macro 
>>> '_GL_ATTRIBUTE_FALLTHROUGH'
>>> # define _GL_ATTRIBUTE_FALLTHROUGH __attribute__ ((__fallthrough__))
> 
> My guess is that the preprocessor expands __has_attribute (__fallthrough__) 
> to 1 but the attribute does not actually work. Is that the case?

has_attribute is indeed 1.

> Looking at the Clang 3.4 source code, I don't see how that could be, as 
> clang-3.4/include/clang/Basic/Attr.td lists the clang::fallthrough attribute 
> for CXX11 but not for C. Possibly I'm misunderstanding that file, but just to 
> check, does your preprocessor version match your compiler version? or are you 
> compiling with clang++?

No, this is the genuine C compiler of that version.  As provided by Ubuntu.

> It might not worth worrying about this warning glitch for such an old 
> compiler, as you can just ignore the bogus warnings.

I sure can do that, but that would require to check at configure time what 
compiler I'm using to enable the warning or not.  Or to make it not covered by 
-Werror, indeed.  Except that I want it to be really an error with modern 
compilers, so that the CI would catch these issues in Bison's own files.  So I 
guess we are back to the discussion about applying a host project compiler 
flags to a guest gnulib.  In the current case, it would perfectly work for me.

So I should look into it, and use different warning flags for host and guest 
files, indeed.

Cheers!

Portability issues with scractch_buffer

[Akim Demaille]

Hi all,

Clang 3.3 and 3.4 cannot compile the new scratch-buffer module.  On Bison's CI 
(Clang 3.4: https://api.travis-ci.org/v3/job/755133481/log.txt), there is:

> make[2]: Entering directory `/home/travis/build/bison-3.7.4.285-a7d1a/_build'
>   CC   lib/lib_libbison_a-bitsetv.o
>   CC   lib/lib_libbison_a-c-ctype.o
>   CC   lib/lib_libbison_a-c-strcasecmp.o
>   CC   lib/lib_libbison_a-c-strncasecmp.o
>   CC   lib/lib_libbison_a-canonicalize.o
> In file included from ../lib/canonicalize.c:31:
> In file included from ../lib/scratch_buffer.h:28:
> ../lib/malloc/scratch_buffer.h:69:11: error: unknown type name 'max_align_t'
>   union { max_align_t __align; char __c[1024]; } __space;
>   ^
> ../lib/canonicalize.c:237:15: warning: declaration does not declare anything 
> [-Wmissing-declarations]
>   FALLTHROUGH;
>   ^~~
> ../lib/attribute.h:142:21: note: expanded from macro 'FALLTHROUGH'
> #define FALLTHROUGH _GL_ATTRIBUTE_FALLTHROUGH
> ^
> ./lib/config.h:2016:36: note: expanded from macro '_GL_ATTRIBUTE_FALLTHROUGH'
> # define _GL_ATTRIBUTE_FALLTHROUGH __attribute__ ((__fallthrough__))
>^
> 1 warning and 1 error generated.
> make[2]: *** [lib/lib_libbison_a-canonicalize.o] Error 1
> make[2]: *** Waiting for unfinished jobs
>   CC   lib/lib_libbison_a-careadlinkat.o
> make[2]: Leaving directory `/home/travis/build/bison-3.7.4.285-a7d1a/_build'
> make[1]: *** [all-recursive] Error 1
> make[1]: Leaving directory `/home/travis/build/bis

Cheers!

Re: mention the 'bitset' module on planet.gnu.org?

[Akim Demaille]

Hi Bruno,

> Le 3 janv. 2021 à 03:19, Bruno Haible  a écrit :
> 
> Hi Akim,
> 
> You might have noticed that we are currently publishing a news item per week
> about Gnulib in https://savannah.gnu.org/news/?group=gnulib ; they get
> propagated to http://planet.gnu.org/ .
> 
> Would you want to write a short text about the 'bitset' module, for
> publication on 2021-01-06 or 2021-01-20?
> 
> The audience is someone who may have heard about Gnulib, but is not yet
> using it.

Here's my proposal.  Feel free to edit it at will.

Cheers!

# A set of bitset implementations

Gnulib features bitset, a module to support operations of lists of bits.

Its API is rich, and includes:
- all the expected operations on single bit (set, toggle, test, etc.);
- all the traditional binary bitwise operators (and, or, xor), often in two 
flavors (return new values, or perform in place);
- some useful ternary operations, such as ((a ∧ b) ∨ c), ((a ∧ ¬b) ∨ c), etc.  
Also in two flavors;
- many predicates (empty, equal, intersects, disjoint, subset and so forth);
- and of course, object creation, destruction, printing, iteration, reverse 
iteration, etc.

The following example, taken from Bison, shows the bitset module in action.  
It's a fix-point computation of `N`, a bitset of the "useful" symbols (a symbol 
is useful if it can actually correspond to a piece of text.  Think for instance 
to `a: a b; b: a;`, `a` and `b` are useless).

```
static void
useless_nonterminals (bitset N, bitset P)
{
  /* N is the bitset as built.  Np is set being built this iteration. 
 P is bitset of all productions which have a RHS all in N.  */
  bitset Np = bitset_create (nnterms, BITSET_FIXED);
  while (true)
{
  bitset_copy (Np, N);
  for (rule_number r = 0; r < nrules; ++r)
if (!bitset_test (P, r)
&& useful_production (r, N))
  {
bitset_set (Np, rules[r].lhs->number);
bitset_set (P, r);
  }
  if (bitset_equal_p (N, Np))
break;
  bitset_swap (N, Np);
}
  bitset_free (N);
  N = Np;
}
```

Like several other gnulib modules, bitset's API actually sits on top of several 
implementations, with different performance profiles.  Indeed bitsets can have 
a fixed size, or being resizable; they can be tailored for dense sets (think of 
an array of bits), or sparse (think of list of bits, or alternatively an table 
of segments of dense bits).  The module also includes a dedicated 
implementation for small bitsets, fitting in machine words, which is 
automatically selects when appropriate.  It even features another 
implementation, stats, which wraps a "genuine" implementation to gather 
statistical data about the use of the bitsets, to help you tune your use of 
bitset.

All this in a very transparent manner: an argument provided to the constructor 
(see `BITSET_FIXED` in the example above).

Gnulib hosts another module, bitsetv, which uses the bitset module to provide 
support for matrices of bits.

Both modules were crafted in 2002 by Michael Hayes for GCC 
(https://gcc.gnu.org/ml/gcc/2002-01/msg00825.html), but, to the best of our 
knowledge, it was never adopted there.  However, the Bison team imported into 
Bison and maintained it over the years, and later contributed it to gnulib.

[PATCH 6/7] bitset: use integrer_length in list implementation

[Akim Demaille]

* lib/bitset/list.c (lbitset_list_reverse): Use
BITSET_FOR_EACH_BIT_REVERSE.
---
 ChangeLog |  6 ++
 lib/bitset/list.c | 26 --
 2 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 37024c15c..c0013145d 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2020-11-29  Akim Demaille  
+
+   bitset: use integrer_length in list implementation
+   * lib/bitset/list.c (lbitset_list_reverse): Use
+   BITSET_FOR_EACH_BIT_REVERSE.
+
 2020-11-29  Akim Demaille  
 
bitset: use integrer_length in vector implementation
diff --git a/lib/bitset/list.c b/lib/bitset/list.c
index e931fe1ca..9a5d48282 100644
--- a/lib/bitset/list.c
+++ b/lib/bitset/list.c
@@ -604,25 +604,23 @@ lbitset_list_reverse (bitset bset, bitset_bindex *list,
   bitset_word *srcp = elt->words;
 
   for (; (windex - elt->index) < LBITSET_ELT_WORDS;
-   windex--, bitoff -= BITSET_WORD_BITS,
- bitcnt = BITSET_WORD_BITS - 1)
+   windex--)
 {
-  bitset_word word =
-srcp[windex - elt->index] << (BITSET_WORD_BITS - 1 - bitcnt);
-
-  for (; word; bitcnt--)
+  bitset_word word = srcp[windex - elt->index];
+  if (bitcnt + 1 < BITSET_WORD_BITS)
+/* We're starting in the middle of a word: smash bits to ignore.  
*/
+word &= ((bitset_word) 1 << (bitcnt + 1)) - 1;
+  BITSET_FOR_EACH_BIT_REVERSE(pos, word)
 {
-  if (word & BITSET_MSB)
+  list[count++] = bitoff + pos;
+  if (count >= num)
 {
-  list[count++] = bitoff + bitcnt;
-  if (count >= num)
-{
-  *next = n_bits - (bitoff + bitcnt);
-  return count;
-}
+  *next = n_bits - (bitoff + pos);
+  return count;
 }
-  word <<= 1;
 }
+  bitoff -= BITSET_WORD_BITS;
+  bitcnt = BITSET_WORD_BITS - 1;
 }
 
   elt = elt->prev;
-- 
2.29.2

[PATCH 5/7] bitset: use integrer_length in vector implementation

[Akim Demaille]

* lib/bitset/array.c (vbitset_list_reverse): Use
BITSET_FOR_EACH_BIT_REVERSE.
---
 ChangeLog   |  6 ++
 lib/bitset/vector.c | 23 +++
 2 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index ecf0fc054..37024c15c 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2020-11-29  Akim Demaille  
+
+   bitset: use integrer_length in vector implementation
+   * lib/bitset/array.c (vbitset_list_reverse): Use
+   BITSET_FOR_EACH_BIT_REVERSE.
+
 2020-11-29  Akim Demaille  
 
bitset: use integrer_length in array implementation
diff --git a/lib/bitset/vector.c b/lib/bitset/vector.c
index 4c0c9dbfd..9f1c08e77 100644
--- a/lib/bitset/vector.c
+++ b/lib/bitset/vector.c
@@ -152,11 +152,11 @@ static bitset_bindex
 vbitset_list_reverse (bitset src, bitset_bindex *list,
   bitset_bindex num, bitset_bindex *next)
 {
+  /* FIXME: almost a duplicate of abitset_list_reverse.  Factor?  */
+  bitset_bindex rbitno = *next;
   bitset_word *srcp = VBITSET_WORDS (src);
   bitset_bindex n_bits = BITSET_SIZE_ (src);
 
-  bitset_bindex rbitno = *next;
-
   /* If num is 1, we could speed things up with a binary search
  of the word of interest.  */
 
@@ -173,19 +173,18 @@ vbitset_list_reverse (bitset src, bitset_bindex *list,
 
   do
 {
-  bitset_word word = srcp[windex] << (BITSET_WORD_BITS - 1 - bitcnt);
-  for (; word; bitcnt--)
+  bitset_word word = srcp[windex];
+  if (bitcnt + 1 < BITSET_WORD_BITS)
+/* We're starting in the middle of a word: smash bits to ignore.  */
+word &= ((bitset_word) 1 << (bitcnt + 1)) - 1;
+  BITSET_FOR_EACH_BIT_REVERSE(pos, word)
 {
-  if (word & BITSET_MSB)
+  list[count++] = bitoff + pos;
+  if (count >= num)
 {
-  list[count++] = bitoff + bitcnt;
-  if (count >= num)
-{
-  *next = n_bits - (bitoff + bitcnt);
-  return count;
-}
+  *next = n_bits - (bitoff + pos);
+  return count;
 }
-  word <<= 1;
 }
   bitoff -= BITSET_WORD_BITS;
   bitcnt = BITSET_WORD_BITS - 1;
-- 
2.29.2

[PATCH 7/7] bitset: use integrer_length in table implementation

[Akim Demaille]

* lib/bitset/table.c (tbitset_list_reverse): Use
BITSET_FOR_EACH_BIT_REVERSE.
---
 ChangeLog  |  6 ++
 lib/bitset/table.c | 19 +--
 2 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index c0013145d..b7c97e437 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2020-11-29  Akim Demaille  
+
+   bitset: use integrer_length in table implementation
+   * lib/bitset/table.c (tbitset_list_reverse): Use
+   BITSET_FOR_EACH_BIT_REVERSE.
+
 2020-11-29  Akim Demaille  
 
bitset: use integrer_length in list implementation
diff --git a/lib/bitset/table.c b/lib/bitset/table.c
index 48c88e02b..16781958a 100644
--- a/lib/bitset/table.c
+++ b/lib/bitset/table.c
@@ -562,19 +562,18 @@ tbitset_list_reverse (bitset bset, bitset_bindex *list,
 
   do
 {
-  for (bitset_word word = srcp[woffset] << (BITSET_WORD_BITS - 1 - 
bitcnt);
-   word; bitcnt--)
+  bitset_word word = srcp[woffset];
+  if (bitcnt + 1 < BITSET_WORD_BITS)
+/* We're starting in the middle of a word: smash bits to 
ignore.  */
+word &= ((bitset_word) 1 << (bitcnt + 1)) - 1;
+  BITSET_FOR_EACH_BIT_REVERSE(pos, word)
 {
-  if (word & BITSET_MSB)
+  list[count++] = bitoff + pos;
+  if (count >= num)
 {
-  list[count++] = bitoff + bitcnt;
-  if (count >= num)
-{
-  *next = n_bits - (bitoff + bitcnt);
-  return count;
-}
+  *next = n_bits - (bitoff + pos);
+  return count;
 }
-  word <<= 1;
 }
   bitoff -= BITSET_WORD_BITS;
   bitcnt = BITSET_WORD_BITS - 1;
-- 
2.29.2

[PATCH 4/7] bitset: use integrer_length in array implementation

[Akim Demaille]

* modules/bitset (Depends-on): Add integrer_length_l.
* lib/bitset/base.h (bitset_fls_, BITSET_FOR_EACH_BIT_REVERSE): New.
* lib/bitset/array.c (abitset_list_reverse): Use it.
---
 ChangeLog  |  7 +++
 lib/bitset/array.c | 19 +--
 lib/bitset/base.h  | 21 +
 modules/bitset |  1 +
 4 files changed, 38 insertions(+), 10 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 48d6191dd..ecf0fc054 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,10 @@
+2020-11-29  Akim Demaille  
+
+   bitset: use integrer_length in array implementation
+   * modules/bitset (Depends-on): Add integrer_length_l.
+   * lib/bitset/base.h (bitset_fls_, BITSET_FOR_EACH_BIT_REVERSE): New.
+   * lib/bitset/array.c (abitset_list_reverse): Use it.
+
 2020-11-29  Akim Demaille  
 
bitset: style: use consistent names
diff --git a/lib/bitset/array.c b/lib/bitset/array.c
index 0e90b6b1f..34773c787 100644
--- a/lib/bitset/array.c
+++ b/lib/bitset/array.c
@@ -143,19 +143,18 @@ abitset_list_reverse (bitset src, bitset_bindex *list,
 
   do
 {
-  bitset_word word = srcp[windex] << (BITSET_WORD_BITS - 1 - bitcnt);
-  for (; word; bitcnt--)
+  bitset_word word = srcp[windex];
+  if (bitcnt + 1 < BITSET_WORD_BITS)
+/* We're starting in the middle of a word: smash bits to ignore.  */
+word &= ((bitset_word) 1 << (bitcnt + 1)) - 1;
+  BITSET_FOR_EACH_BIT_REVERSE(pos, word)
 {
-  if (word & BITSET_MSB)
+  list[count++] = bitoff + pos;
+  if (count >= num)
 {
-  list[count++] = bitoff + bitcnt;
-  if (count >= num)
-{
-  *next = n_bits - (bitoff + bitcnt);
-  return count;
-}
+  *next = n_bits - (bitoff + pos);
+  return count;
 }
-  word <<= 1;
 }
   bitoff -= BITSET_WORD_BITS;
   bitcnt = BITSET_WORD_BITS - 1;
diff --git a/lib/bitset/base.h b/lib/bitset/base.h
index a124b0df4..64188ddb5 100644
--- a/lib/bitset/base.h
+++ b/lib/bitset/base.h
@@ -27,6 +27,7 @@
 #include  /* ffsl */
 
 #include "attribute.h"
+#include "integer_length.h"
 #include "xalloc.h"
 
 /* Currently we support five flavours of bitsets:
@@ -286,6 +287,14 @@ if (!BITSET_COMPATIBLE_ (DST, SRC1) || !BITSET_COMPATIBLE_ 
(DST, SRC2) \
0 <= Pos;\
Word ^= 1UL << Pos, Pos = bitset_ffs_ (Word))
 
+/* Iterate right to left over each set bit of WORD.
+   Each iteration sets POS to the 0-based index of the next set bit in WORD.
+   Repeatedly resets bits in WORD in place until it's null.  */
+#define BITSET_FOR_EACH_BIT_REVERSE(Pos, Word)  \
+  for (int Pos = bitset_fls_ (Word);\
+   0 <= Pos;\
+   Word ^= 1UL << Pos, Pos = bitset_fls_ (Word))
+
 /* Private functions for bitset implementations.  */
 
 bool bitset_toggle_ (bitset, bitset_bindex);
@@ -316,4 +325,16 @@ int bitset_ffs_ (bitset_word word)
   return ffsl ((long) word) - 1;
 }
 
+#include 
+#include 
+
+/* Last set bit in WORD.
+   Indexes start at 0, return -1 if WORD is null. */
+static inline
+int bitset_fls_ (bitset_word word)
+{
+  int res = integer_length_l (word) - 1;
+  return res;
+}
+
 #endif /* _BBITSET_H  */
diff --git a/modules/bitset b/modules/bitset
index 2516c2861..8c9cb7cc8 100644
--- a/modules/bitset
+++ b/modules/bitset
@@ -22,6 +22,7 @@ c99
 ffsl
 fopen-gnu
 gettext-h
+integer_length_l
 obstack
 xalloc
 
-- 
2.29.2

[PATCH 3/7] bitset: style: use consistent names

[Akim Demaille]

* bitset/list.c (lbitset_list_reverse): Rename 'bcount' as 'bitcnt',
and 'boffset' as 'bitoff', for consistency with the other
implementations.
* bitset/table.c (tbitset_list_reverse): Likewise.
---
 ChangeLog  |  8 
 lib/bitset/list.c  | 24 
 lib/bitset/table.c | 20 ++--
 3 files changed, 30 insertions(+), 22 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 7f83f5e1d..48d6191dd 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@
+2020-11-29  Akim Demaille  
+
+   bitset: style: use consistent names
+   * bitset/list.c (lbitset_list_reverse): Rename 'bcount' as 'bitcnt',
+   and 'boffset' as 'bitoff', for consistency with the other
+   implementations.
+   * bitset/table.c (tbitset_list_reverse): Likewise.
+
 2020-11-29  Akim Demaille  
 
bitset: style: sort header
diff --git a/lib/bitset/list.c b/lib/bitset/list.c
index 96f163087..e931fe1ca 100644
--- a/lib/bitset/list.c
+++ b/lib/bitset/list.c
@@ -580,21 +580,21 @@ lbitset_list_reverse (bitset bset, bitset_bindex *list,
   if (!elt)
 return 0;
 
-  unsigned bcount;
+  unsigned bitcnt;
   if (windex >= elt->index + LBITSET_ELT_WORDS)
 {
   /* We are trying to start in no-mans land so start
  at end of current elt.  */
-  bcount = BITSET_WORD_BITS - 1;
+  bitcnt = BITSET_WORD_BITS - 1;
   windex = elt->index + LBITSET_ELT_WORDS - 1;
 }
   else
 {
-  bcount = bitno % BITSET_WORD_BITS;
+  bitcnt = bitno % BITSET_WORD_BITS;
 }
 
   bitset_bindex count = 0;
-  bitset_bindex boffset = windex * BITSET_WORD_BITS;
+  bitset_bindex bitoff = windex * BITSET_WORD_BITS;
 
   /* If num is 1, we could speed things up with a binary search
  of the word of interest.  */
@@ -604,20 +604,20 @@ lbitset_list_reverse (bitset bset, bitset_bindex *list,
   bitset_word *srcp = elt->words;
 
   for (; (windex - elt->index) < LBITSET_ELT_WORDS;
-   windex--, boffset -= BITSET_WORD_BITS,
- bcount = BITSET_WORD_BITS - 1)
+   windex--, bitoff -= BITSET_WORD_BITS,
+ bitcnt = BITSET_WORD_BITS - 1)
 {
   bitset_word word =
-srcp[windex - elt->index] << (BITSET_WORD_BITS - 1 - bcount);
+srcp[windex - elt->index] << (BITSET_WORD_BITS - 1 - bitcnt);
 
-  for (; word; bcount--)
+  for (; word; bitcnt--)
 {
   if (word & BITSET_MSB)
 {
-  list[count++] = boffset + bcount;
+  list[count++] = bitoff + bitcnt;
   if (count >= num)
 {
-  *next = n_bits - (boffset + bcount);
+  *next = n_bits - (bitoff + bitcnt);
   return count;
 }
 }
@@ -629,11 +629,11 @@ lbitset_list_reverse (bitset bset, bitset_bindex *list,
   if (elt)
 {
   windex = elt->index + LBITSET_ELT_WORDS - 1;
-  boffset = windex * BITSET_WORD_BITS;
+  bitoff = windex * BITSET_WORD_BITS;
 }
 }
 
-  *next = n_bits - (boffset + 1);
+  *next = n_bits - (bitoff + 1);
   return count;
 }
 
diff --git a/lib/bitset/table.c b/lib/bitset/table.c
index 3643b6074..48c88e02b 100644
--- a/lib/bitset/table.c
+++ b/lib/bitset/table.c
@@ -550,8 +550,8 @@ tbitset_list_reverse (bitset bset, bitset_bindex *list,
   /* If num is 1, we could speed things up with a binary search
  of the word of interest.  */
   bitset_bindex count = 0;
-  unsigned bcount = bitno % BITSET_WORD_BITS;
-  bitset_bindex boffset = windex * BITSET_WORD_BITS;
+  unsigned bitcnt = bitno % BITSET_WORD_BITS;
+  bitset_bindex bitoff = windex * BITSET_WORD_BITS;
 
   do
 {
@@ -562,32 +562,32 @@ tbitset_list_reverse (bitset bset, bitset_bindex *list,
 
   do
 {
-  for (bitset_word word = srcp[woffset] << (BITSET_WORD_BITS - 1 - 
bcount);
-   word; bcount--)
+  for (bitset_word word = srcp[woffset] << (BITSET_WORD_BITS - 1 - 
bitcnt);
+   word; bitcnt--)
 {
   if (word & BITSET_MSB)
 {
-  list[count++] = boffset + bcount;
+  list[count++] = bitoff + bitcnt;
   if (count >= num)
 {
-  *next = n_bits - (boffset + bcount);
+  *next = n_bits - (bitoff + bitcnt);
   return count;
 }
 }
   word <<= 1;
 }
-  boffset -= BITSET_WORD_BITS;
-  bcount = BITSET_WORD_BITS - 1;
+  bitoff -= BITSET_WORD_BITS;
+  bitcnt = BITSET_WORD_BITS - 1;
 }
   while (woffset--);
 }
 
   woff

[PATCH 2/7] bitset: style: sort header

[Akim Demaille]

* lib/bitset/base.h (bitset_ffs): Rename as...
(bitset_ffs_): this.
(bitset_ffs_, BITSET_FOR_EACH_BIT): Move to better places.
---
 ChangeLog |  7 +++
 lib/bitset/base.h | 31 +++
 2 files changed, 22 insertions(+), 16 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 67576a51f..7f83f5e1d 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,10 @@
+2020-11-29  Akim Demaille  
+
+   bitset: style: sort header
+   * lib/bitset/base.h (bitset_ffs): Rename as...
+   (bitset_ffs_): this.
+   (bitset_ffs_, BITSET_FOR_EACH_BIT): Move to better places.
+
 2020-11-29  Akim Demaille  
 
bitset: tests: check BITSET_FOR_EACH_REVERSE
diff --git a/lib/bitset/base.h b/lib/bitset/base.h
index 50569a0fc..a124b0df4 100644
--- a/lib/bitset/base.h
+++ b/lib/bitset/base.h
@@ -53,14 +53,6 @@ extern const char * const bitset_type_names[];
 typedef unsigned long bitset_word;
 #define BITSET_WORD_BITS ((unsigned) (CHAR_BIT * sizeof (bitset_word)))
 
-/* Iterate over each set bit of WORD.
-   Each iteration sets POS to the 0-based index of the next set bit in WORD.
-   Repeatedly resets bits in WORD in place until it's null.  */
-#define BITSET_FOR_EACH_BIT(Pos, Word)  \
-  for (int Pos = bitset_ffs (Word); \
-   0 <= Pos;\
-   Word ^= 1UL << Pos, Pos = bitset_ffs (Word))
-
 /* Bit index.  In theory we might need a type wider than size_t, but
in practice we lose at most a factor of CHAR_BIT by going with
size_t, and that is good enough.  If this type is changed to be
@@ -69,14 +61,6 @@ typedef unsigned long bitset_word;
The bit and word index types must be unsigned.  */
 typedef size_t bitset_bindex;
 
-/* First first set bit in WORD.
-   Indexes start at 0, return -1 if WORD is null. */
-static inline
-int bitset_ffs (bitset_word word)
-{
-  return ffsl ((long) word) - 1;
-}
-
 /* Word index.  */
 typedef size_t bitset_windex;
 
@@ -294,6 +278,13 @@ if (!BITSET_COMPATIBLE_ (DST, SRC1) || !BITSET_COMPATIBLE_ 
(DST, SRC2) \
 #define BITSET_LIST_REVERSE_(BSET, LIST, NUM, NEXT) \
  (BSET)->b.vtable->list_reverse (BSET, LIST, NUM, NEXT)
 
+/* Iterate left to right over each set bit of WORD.
+   Each iteration sets POS to the 0-based index of the next set bit in WORD.
+   Repeatedly resets bits in WORD in place until it's null.  */
+#define BITSET_FOR_EACH_BIT(Pos, Word)  \
+  for (int Pos = bitset_ffs_ (Word);\
+   0 <= Pos;\
+   Word ^= 1UL << Pos, Pos = bitset_ffs_ (Word))
 
 /* Private functions for bitset implementations.  */
 
@@ -317,4 +308,12 @@ void bitset_or_and_ (bitset, bitset, bitset, bitset);
 
 bool bitset_or_and_cmp_ (bitset, bitset, bitset, bitset);
 
+/* First set bit in WORD.
+   Indexes start at 0, return -1 if WORD is null. */
+static inline
+int bitset_ffs_ (bitset_word word)
+{
+  return ffsl ((long) word) - 1;
+}
+
 #endif /* _BBITSET_H  */
-- 
2.29.2

[PATCH 1/7] bitset: tests: check BITSET_FOR_EACH_REVERSE

[Akim Demaille]

* tests/test-bitset.c (compare, check_zero, check_one_bit, check_ones):
Check BITSET_FOR_EACH_REVERSE.
---
 ChangeLog   |  6 ++
 tests/test-bitset.c | 39 +++
 2 files changed, 45 insertions(+)

diff --git a/ChangeLog b/ChangeLog
index fa21e9cd2..67576a51f 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2020-11-29  Akim Demaille  
+
+   bitset: tests: check BITSET_FOR_EACH_REVERSE
+   * tests/test-bitset.c (compare, check_zero, check_one_bit, check_ones):
+   Check BITSET_FOR_EACH_REVERSE.
+
 2020-11-28  Bruno Haible  
 
asyncsafe-spin: Fix compilation error with GCC on 32-bit SPARC.
diff --git a/tests/test-bitset.c b/tests/test-bitset.c
index 6fa656a22..ab3abac13 100644
--- a/tests/test-bitset.c
+++ b/tests/test-bitset.c
@@ -200,6 +200,36 @@ compare (enum bitset_attr a, enum bitset_attr b)
   }
   }
 
+  /* FOR_EACH_REVERSE.  */
+  {
+bitset_iterator iter;
+bitset_bindex j;
+bitset_bindex first = bitset_first (bdst);
+bitset_bindex last  = bitset_last (bdst);
+bool seen_first = false;
+bool seen_last = false;
+BITSET_FOR_EACH_REVERSE (iter, bdst, j, 0)
+  {
+ASSERT (first <= j && j <= last);
+ASSERT (bitset_test (bdst, j));
+if (j == first)
+  seen_first = true;
+if (j == last)
+  seen_last = true;
+  }
+if (first == BITSET_BINDEX_MAX)
+  {
+ASSERT (!seen_first);
+ASSERT (!seen_last);
+  }
+else
+  {
+ASSERT (seen_first);
+ASSERT (seen_last);
+  }
+  }
+
+
   /* resize.
 
  ARRAY bitsets cannot be resized.  */
@@ -247,6 +277,8 @@ check_zero (bitset bs)
 bitset_bindex i;
 BITSET_FOR_EACH (iter, bs, i, 0)
   ASSERT (0);
+BITSET_FOR_EACH_REVERSE (iter, bs, i, 0)
+  ASSERT (0);
   }
 }
 
@@ -276,6 +308,9 @@ check_one_bit (bitset bs, int bitno)
 bitset_bindex i;
 BITSET_FOR_EACH (iter, bs, i, 0)
   ASSERT (i == bitno);
+
+BITSET_FOR_EACH_REVERSE (iter, bs, i, 0)
+  ASSERT (i == bitno);
   }
 }
 
@@ -304,6 +339,10 @@ check_ones (bitset bs)
 bitset_bindex count = 0;
 BITSET_FOR_EACH (iter, bs, i, 0)
   ASSERT (i == count++);
+ASSERT (count == size);
+BITSET_FOR_EACH_REVERSE (iter, bs, i, 0)
+  ASSERT (i == --count);
+ASSERT (count == 0);
   }
 }
 
-- 
2.29.2

[PATCH 0/7] bitset: use integrer_length in reverse iterations

[Akim Demaille]

This is the continuation of
https://lists.gnu.org/archive/html/bug-gnulib/2020-11/msg00052.html
and
https://lists.gnu.org/archive/html/bug-gnulib/2020-11/msg00075.html
which replaced bit-by-bit forward iterations with uses of ffs, as
suggested by Bruno.  This time, we replace bit-by-bit _backward_
iterations with uses of integrer_length.

The test suite passes with asan and ubsan.  Bison's test suite passes,
but this is irrelevant, since Bison does not depend on reverse
iteration.

I'll push it tomorrow, unless told otherwise.

Cheers!

Akim Demaille (7):
  bitset: tests: check BITSET_FOR_EACH_REVERSE
  bitset: style: sort header
  bitset: style: use consistent names
  bitset: use integrer_length in array implementation
  bitset: use integrer_length in vector implementation
  bitset: use integrer_length in list implementation
  bitset: use integrer_length in table implementation

 ChangeLog   | 46 +++
 lib/bitset/array.c  | 19 -
 lib/bitset/base.h   | 52 +++--
 lib/bitset/list.c   | 38 -
 lib/bitset/table.c  | 31 +--
 lib/bitset/vector.c | 23 ++--
 modules/bitset  |  1 +
 tests/test-bitset.c | 39 ++
 8 files changed, 175 insertions(+), 74 deletions(-)

-- 
2.29.2

Re: bitpack search in bitsets

[Akim Demaille]

Hi Bruno,

> Le 18 nov. 2020 à 10:53, Bruno Haible  a écrit :
> 
>> Conversely: couldn't ssfmalloc sit on top of bitset?
> 
> Well, in ssfmalloc it is important to use the available space well. When
> I use a bitset of 256 bits in order to describe which of the 16-byte blocks
> of a 4096-bytes page are free, this requires 8 32-bit words. Now adding
> 5 (32-bit or 64-bit) words as meta-information, as defined per these
> type definitions: [...]
> is not really desirable. It would be only 1% waste, but the percents add up.
> 
> In other words, the 'bitset' module - while generally good for applications -
> is one level too complex for this particular use-case.

It makes a lot of sense.  Thanks for explaining!

Cheers.

Re: [PATCH 0/6] bitset: more conversions to ffs

[Akim Demaille]

Hi Bruno,

So I installed these commits.

> Le 20 nov. 2020 à 00:05, Bruno Haible  a écrit :
> 
> Akim Demaille wrote:
>>> Gnulib has modules integer_length, integer_length_l, integer_length_ll.
>> 
>> Oh, yes.  Why these names?
> 
> The name 'integer-length' comes from ANSI Common Lisp [1].
> [...]
> ANSI Common Lisp predates FreeBSD 5.3 by 15 years :-)

Thanks for the reference!  Cheers!

Re: [PATCH 0/6] bitset: more conversions to ffs

[Akim Demaille]

Hi Bruno,

> Le 19 nov. 2020 à 16:04, Bruno Haible  a écrit :
> 
> Hi Akim,
> 
>> When gnulib have fls* support, we will be able do the same for
>> bitset_list_reverse
> 
> Gnulib has modules integer_length, integer_length_l, integer_length_ll.

Oh, yes.  Why these names?  "fls" feels more "natural" in front of fss.
But just as cryptic indeed.

I can see it is not POSIX, but

https://www.freebsd.org/cgi/man.cgi?query=fls=3=FreeBSD+7.1-RELEASE

> That sounds like what you are asking for.

Too bad :( I was happy being done with that :)

Thanks!

[PATCH 6/6] bitset: exercise the stats too

[Akim Demaille]

* tests/test-bitset.c: Display the stats at the end of the test.
* lib/bitset/stats.c (bitset_log_histogram_print): When diplaying the
last bin, display "256-..." rather that "256-511", since the last bin
does count item greater than or equal to 256.
---
 lib/bitset/stats.c  | 8 +++-
 tests/test-bitset.c | 4 
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/lib/bitset/stats.c b/lib/bitset/stats.c
index 5bd44c06a..df9264285 100644
--- a/lib/bitset/stats.c
+++ b/lib/bitset/stats.c
@@ -154,13 +154,19 @@ bitset_log_histogram_print (FILE *file, const char *name, 
const char *msg,
   fprintf (file, "%*d\t%8u (%5.1f%%)\n",
max_width, i, bins[i], 100.0 * bins[i] / total);
 
-for (; i < n_bins; i++)
+for (; i < n_bins - 1; i++)
   fprintf (file, "%*lu-%lu\t%8u (%5.1f%%)\n",
max_width - ((unsigned) (0.30103 * (i) + 0.) + 1),
1UL << (i - 1),
(1UL << i) - 1,
bins[i],
(100.0 * bins[i]) / total);
+
+fprintf (file, "%*lu-...\t%8u (%5.1f%%)\n",
+ max_width - ((unsigned) (0.30103 * (i) + 0.) + 1),
+ 1UL << (i - 1),
+ bins[i],
+ (100.0 * bins[i]) / total);
   }
 }
 
diff --git a/tests/test-bitset.c b/tests/test-bitset.c
index 9a2d7c521..6fa656a22 100644
--- a/tests/test-bitset.c
+++ b/tests/test-bitset.c
@@ -358,6 +358,8 @@ check_attributes (enum bitset_attr attr, int nbits)
 
 int main (void)
 {
+  bitset_stats_enable ();
+
   for (int i = 0; i < 4; ++i)
 {
   /* table bitsets have elements that store 256 bits.  bitset_list
@@ -382,5 +384,7 @@ int main (void)
   compare (BITSET_VARIABLE, BITSET_SPARSE);
   compare (BITSET_VARIABLE, BITSET_FRUGAL);
   compare (BITSET_VARIABLE, BITSET_GREEDY);
+
+  bitset_stats_dump (stderr);
   return 0;
 }
-- 
2.29.2

[PATCH 6/6] bitset: tests: exercise the stats too

[Akim Demaille]

* tests/test-bitset.c: Display the stats at the end of the test.
* lib/bitset/stats.c (bitset_log_histogram_print): When diplaying the
last bin, display "256-..." rather that "256-511", since the last bin
does count item greater than or equal to 256.
---
 ChangeLog   | 9 +
 lib/bitset/stats.c  | 8 +++-
 tests/test-bitset.c | 4 
 3 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/ChangeLog b/ChangeLog
index cd595f41d..55c4e08e8 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,12 @@
+2020-11-19  Akim Demaille  
+
+   bitset: tests: exercise the stats too
+
+   * tests/test-bitset.c: Display the stats at the end of the test.
+   * lib/bitset/stats.c (bitset_log_histogram_print): When diplaying the
+   last bin, display "256-..." rather that "256-511", since the last bin
+   does count item greater than or equal to 256.
+
 2020-11-19  Akim Demaille  
 
bitset: tests: try harder to break it
diff --git a/lib/bitset/stats.c b/lib/bitset/stats.c
index 5bd44c06a..df9264285 100644
--- a/lib/bitset/stats.c
+++ b/lib/bitset/stats.c
@@ -154,13 +154,19 @@ bitset_log_histogram_print (FILE *file, const char *name, 
const char *msg,
   fprintf (file, "%*d\t%8u (%5.1f%%)\n",
max_width, i, bins[i], 100.0 * bins[i] / total);
 
-for (; i < n_bins; i++)
+for (; i < n_bins - 1; i++)
   fprintf (file, "%*lu-%lu\t%8u (%5.1f%%)\n",
max_width - ((unsigned) (0.30103 * (i) + 0.) + 1),
1UL << (i - 1),
(1UL << i) - 1,
bins[i],
(100.0 * bins[i]) / total);
+
+fprintf (file, "%*lu-...\t%8u (%5.1f%%)\n",
+ max_width - ((unsigned) (0.30103 * (i) + 0.) + 1),
+ 1UL << (i - 1),
+ bins[i],
+ (100.0 * bins[i]) / total);
   }
 }
 
diff --git a/tests/test-bitset.c b/tests/test-bitset.c
index 9a2d7c521..6fa656a22 100644
--- a/tests/test-bitset.c
+++ b/tests/test-bitset.c
@@ -358,6 +358,8 @@ check_attributes (enum bitset_attr attr, int nbits)
 
 int main (void)
 {
+  bitset_stats_enable ();
+
   for (int i = 0; i < 4; ++i)
 {
   /* table bitsets have elements that store 256 bits.  bitset_list
@@ -382,5 +384,7 @@ int main (void)
   compare (BITSET_VARIABLE, BITSET_SPARSE);
   compare (BITSET_VARIABLE, BITSET_FRUGAL);
   compare (BITSET_VARIABLE, BITSET_GREEDY);
+
+  bitset_stats_dump (stderr);
   return 0;
 }
-- 
2.29.2

[PATCH 5/6] bitset: tests: try harder to break it

[Akim Demaille]

bitset_list (used in bitset_first, bitset_next, bitset_count,
BITSET_FOR_EACH, etc.) uses a cache of size BITSET_LIST_SIZE (1024).
None of our tests current try bitsets bigger than this.

* tests/test-bitset.c (compare): Be ready to use bitsets larger than
BITSET_LIST_SIZE.
(main): Likewise.
While at it, also exercise super small bitsets.
---
 ChangeLog   |  8 
 tests/test-bitset.c | 28 ++--
 2 files changed, 26 insertions(+), 10 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 2bc68b9dc..cd595f41d 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@
+2020-11-19  Akim Demaille  
+
+   bitset: tests: try harder to break it
+   * tests/test-bitset.c (compare): Be ready to use bitsets larger than
+   BITSET_LIST_SIZE.
+   (main): Likewise.
+   While at it, also exercise super small bitsets.
+
 2020-11-19  Akim Demaille  
 
bitset: use ffs where possible in the vector implementation
diff --git a/tests/test-bitset.c b/tests/test-bitset.c
index fc0c6fbe9..9a2d7c521 100644
--- a/tests/test-bitset.c
+++ b/tests/test-bitset.c
@@ -22,8 +22,8 @@
 
 #define RANDOM(n) (rand () % (n))
 
-static
-void assert_bitset_equal (bitset bs1, bitset bs2)
+static void
+assert_bitset_equal (bitset bs1, bitset bs2)
 {
   debug_bitset (bs1);
   debug_bitset (bs2);
@@ -32,8 +32,8 @@ void assert_bitset_equal (bitset bs1, bitset bs2)
 ASSERT (bitset_test (bs1, i) == bitset_test (bs2, i));
 }
 
-static
-void bitset_random (bitset bs)
+static void
+bitset_random (bitset bs)
 {
   for (bitset_bindex i = 0; i < bitset_size (bs); ++i)
 bitset_set (bs, RANDOM (2));
@@ -43,10 +43,12 @@ void bitset_random (bitset bs)
 /* Check various operations on random bitsets with two different
implementations.  */
 
-static
-void compare (enum bitset_attr a, enum bitset_attr b)
+static void
+compare (enum bitset_attr a, enum bitset_attr b)
 {
-  const int nbits = RANDOM (256);
+  /* bitset_list (used in many operations) uses a cache whose size is
+ BITSET_LIST_SIZE */
+  const int nbits = RANDOM (2 * BITSET_LIST_SIZE);
 
   /* Four read only random initial values of type A.  */
   const bitset asrc0 = bitset_create (nbits, a);
@@ -356,10 +358,16 @@ check_attributes (enum bitset_attr attr, int nbits)
 
 int main (void)
 {
-  for (int i = 0; i < 2; ++i)
+  for (int i = 0; i < 4; ++i)
 {
-  /* table bitsets have elements that store 256 bits.  */
-  int nbits = i == 0 ? 32 : 257;
+  /* table bitsets have elements that store 256 bits.  bitset_list
+ (used in many operations) uses a cache whose size is
+ BITSET_LIST_SIZE.  */
+  int nbits =
+i == 0   ? 1
+: i == 1 ? 32
+: i == 2 ? 257
+:  (BITSET_LIST_SIZE + 1);
   check_attributes (BITSET_FIXED,nbits);
   check_attributes (BITSET_VARIABLE, nbits);
   check_attributes (BITSET_DENSE,nbits);
-- 
2.29.2

[PATCH 4/6] bitset: use ffs where possible in the vector implementation

[Akim Demaille]

* lib/bitset/vector.c (vbitset_list): Use BITSET_FOR_EACH_BIT.
---
 ChangeLog   |  5 
 lib/bitset/vector.c | 57 +
 2 files changed, 27 insertions(+), 35 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 951b82b04..2bc68b9dc 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2020-11-19  Akim Demaille  
+
+   bitset: use ffs where possible in the vector implementation
+   * lib/bitset/vector.c (vbitset_list): Use BITSET_FOR_EACH_BIT.
+
 2020-11-19  Akim Demaille  
 
bitset: use ffs where possible in the table implementation
diff --git a/lib/bitset/vector.c b/lib/bitset/vector.c
index fe14d6703..4c0c9dbfd 100644
--- a/lib/bitset/vector.c
+++ b/lib/bitset/vector.c
@@ -204,6 +204,7 @@ static bitset_bindex
 vbitset_list (bitset src, bitset_bindex *list,
   bitset_bindex num, bitset_bindex *next)
 {
+  /* FIXME: almost a duplicate of abitset_list.  Factor?  */
   bitset_windex size = VBITSET_SIZE (src);
   bitset_word *srcp = VBITSET_WORDS (src);
   bitset_bindex bitno = *next;
@@ -211,7 +212,6 @@ vbitset_list (bitset src, bitset_bindex *list,
 
   bitset_windex windex;
   bitset_bindex bitoff;
-  bitset_word word;
 
   if (!bitno)
 {
@@ -242,19 +242,16 @@ vbitset_list (bitset src, bitset_bindex *list,
  on the previous call to this function.  */
 
   bitoff = windex * BITSET_WORD_BITS;
-  word = srcp[windex] >> bitno;
-  for (bitno = bitoff + bitno; word; bitno++)
+  bitset_word word = srcp[windex] >> bitno;
+  bitno = bitoff + bitno;
+  BITSET_FOR_EACH_BIT (pos, word)
 {
-  if (word & 1)
+  list[count++] = bitno + pos;
+  if (count >= num)
 {
-  list[count++] = bitno;
-  if (count >= num)
-{
-  *next = bitno + 1;
-  return count;
-}
+  *next = bitno + pos + 1;
+  return count;
 }
-  word >>= 1;
 }
   windex++;
 }
@@ -263,34 +260,24 @@ vbitset_list (bitset src, bitset_bindex *list,
 
   for (; windex < size; windex++, bitoff += BITSET_WORD_BITS)
 {
-  if (!(word = srcp[windex]))
+  bitset_word word = srcp[windex];
+  if (!word)
 continue;
 
+  /* Is there enough room to avoid checking in each iteration? */
   if ((count + BITSET_WORD_BITS) < num)
-{
-  for (bitno = bitoff; word; bitno++)
-{
-  if (word & 1)
-list[count++] = bitno;
-  word >>= 1;
-}
-}
+BITSET_FOR_EACH_BIT (pos, word)
+  list[count++] = bitoff + pos;
   else
-{
-  for (bitno = bitoff; word; bitno++)
-{
-  if (word & 1)
-{
-  list[count++] = bitno;
-  if (count >= num)
-{
-  *next = bitno + 1;
-  return count;
-}
-}
-  word >>= 1;
-}
-}
+BITSET_FOR_EACH_BIT (pos, word)
+  {
+list[count++] = bitoff + pos;
+if (count >= num)
+  {
+*next = bitoff + pos + 1;
+return count;
+  }
+  }
 }
 
   *next = bitoff;
-- 
2.29.2

[PATCH 2/6] bitset: check empty and full bitsets

[Akim Demaille]

* tests/test-bitset.c (check_zero, check_ones): New.
(check_attributes): Use them.
---
 ChangeLog   |  6 +
 tests/test-bitset.c | 56 +
 2 files changed, 62 insertions(+)

diff --git a/ChangeLog b/ChangeLog
index 88ae15003..5badf0c41 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2020-11-19  Akim Demaille  
+
+   bitset: check empty and full bitsets
+   * tests/test-bitset.c (check_zero, check_ones): New.
+   (check_attributes): Use them.
+
 2020-11-19  Akim Demaille  
 
bitset: be sure to always return a value
diff --git a/tests/test-bitset.c b/tests/test-bitset.c
index 5068ae782..fc0c6fbe9 100644
--- a/tests/test-bitset.c
+++ b/tests/test-bitset.c
@@ -224,12 +224,37 @@ void compare (enum bitset_attr a, enum bitset_attr b)
 }
 
 
+/* Check empty bitsets.  */
+
+static void
+check_zero (bitset bs)
+{
+  fprintf (stderr, "check_zero\n");
+  bitset_zero (bs);
+
+  /* count. */
+  ASSERT (bitset_count (bs) == 0);
+
+  /* first and last */
+  ASSERT (bitset_first (bs) == BITSET_BINDEX_MAX);
+  ASSERT (bitset_last (bs)  == BITSET_BINDEX_MAX);
+
+  /* FOR_EACH.  */
+  {
+bitset_iterator iter;
+bitset_bindex i;
+BITSET_FOR_EACH (iter, bs, i, 0)
+  ASSERT (0);
+  }
+}
+
 /* Exercise on a single-bit values: it's easy to get the handling of
the most significant bit wrong.  */
 
 static void
 check_one_bit (bitset bs, int bitno)
 {
+  fprintf (stderr, "check_one_bit(%d)\n", bitno);
   bitset_zero (bs);
   bitset_set (bs, bitno);
 
@@ -252,6 +277,34 @@ check_one_bit (bitset bs, int bitno)
   }
 }
 
+/* Check full bitsets.  */
+
+static void
+check_ones (bitset bs)
+{
+  fprintf (stderr, "check_ones\n");
+  const bitset_bindex size = bitset_size (bs);
+
+  bitset_ones (bs);
+  debug_bitset (bs);
+
+  /* count. */
+  ASSERT (bitset_count (bs) == size);
+
+  /* first and last */
+  ASSERT (bitset_first (bs) == 0);
+  ASSERT (bitset_last (bs)  == size - 1);
+
+  /* FOR_EACH.  */
+  {
+bitset_iterator iter;
+bitset_bindex i;
+bitset_bindex count = 0;
+BITSET_FOR_EACH (iter, bs, i, 0)
+  ASSERT (i == count++);
+  }
+}
+
 /* Check various operations against expected values for a bitset
having attributes ATTR.  */
 
@@ -287,6 +340,9 @@ check_attributes (enum bitset_attr attr, int nbits)
   bitset_or (bs, bs1, bs2);
   ASSERT (bitset_count (bs) == 6);
 
+  check_zero (bs);
+  check_ones (bs);
+
   /* Exercise on all the single-bit values: it's easy to get the
  handling of the most significant bit wrong.  */
   for (int bitno = 0; bitno < nbits; ++bitno)
-- 
2.29.2

[PATCH 3/6] bitset: use ffs where possible in the table implementation

[Akim Demaille]

* lib/bitset/table.c (tbitset_list): Use BITSET_FOR_EACH_BIT.
---
 ChangeLog  |   5 +++
 lib/bitset/table.c | 104 -
 2 files changed, 31 insertions(+), 78 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 5badf0c41..951b82b04 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2020-11-19  Akim Demaille  
+
+   bitset: use ffs where possible in the table implementation
+   * lib/bitset/table.c (tbitset_list): Use BITSET_FOR_EACH_BIT.
+
 2020-11-19  Akim Demaille  
 
bitset: check empty and full bitsets
diff --git a/lib/bitset/table.c b/lib/bitset/table.c
index d111e0203..3643b6074 100644
--- a/lib/bitset/table.c
+++ b/lib/bitset/table.c
@@ -623,18 +623,14 @@ tbitset_list (bitset bset, bitset_bindex *list,
 {
   bitset_word word = srcp[windex - woffset] >> (bitno % 
BITSET_WORD_BITS);
 
-  for (; word; bitno++)
+  BITSET_FOR_EACH_BIT (pos, word)
 {
-  if (word & 1)
+  list[count++] = bitno + pos;
+  if (count >= num)
 {
-  list[count++] = bitno;
-  if (count >= num)
-{
-  *next = bitno + 1;
-  return count;
-}
+  *next = bitno + pos + 1;
+  return count;
 }
-  word >>= 1;
 }
   bitno = (windex + 1) * BITSET_WORD_BITS;
 }
@@ -649,7 +645,6 @@ tbitset_list (bitset bset, bitset_bindex *list,
 
   for (; eindex < size; eindex++)
 {
-
   tbitset_elt *elt = elts[eindex];
   if (!elt)
 continue;
@@ -666,69 +661,25 @@ tbitset_list (bitset bset, bitset_bindex *list,
 #if TBITSET_ELT_WORDS == 2
   bitset_word word = srcp[0];
   if (word)
-{
-  if (!(word & 0x))
-{
-  word >>= 16;
-  bitno += 16;
-}
-  if (!(word & 0xff))
-{
-  word >>= 8;
-  bitno += 8;
-}
-  for (; word; bitno++)
-{
-  if (word & 1)
-list[count++] = bitno;
-  word >>= 1;
-}
-}
+BITSET_FOR_EACH_BIT (pos, word)
+  list[count++] = bitno + pos;
   windex++;
   bitno = windex * BITSET_WORD_BITS;
 
   word = srcp[1];
   if (word)
-{
-  if (!(word & 0x))
-{
-  word >>= 16;
-  bitno += 16;
-}
-  for (; word; bitno++)
-{
-  if (word & 1)
-list[count++] = bitno;
-  word >>= 1;
-}
-}
+BITSET_FOR_EACH_BIT (pos, word)
+  list[count++] = bitno + pos;
   windex++;
   bitno = windex * BITSET_WORD_BITS;
 #else
   for (int i = 0; i < TBITSET_ELT_WORDS; i++, windex++)
 {
-  bitno = windex * BITSET_WORD_BITS;
-
-  word = srcp[i];
+  bitset_word word = srcp[i];
   if (word)
-{
-  if (!(word & 0x))
-{
-  word >>= 16;
-  bitno += 16;
-}
-  if (!(word & 0xff))
-{
-  word >>= 8;
-  bitno += 8;
-}
-  for (; word; bitno++)
-{
-  if (word & 1)
-list[count++] = bitno;
-  word >>= 1;
-}
-}
+BITSET_FOR_EACH_BIT (pos, word)
+  list[count++] = bitno + pos;
+  bitno = windex * BITSET_WORD_BITS;
 }
 #endif
 }
@@ -736,24 +687,21 @@ tbitset_list (bitset bset, bitset_bindex *list,
 {
   /* Tread more carefully since we need to check
  if array overflows.  */
-
-  for (int i = 0; i < TBITSET_ELT_WORDS; i++, windex++)
+  for (int i = 0; i < TBITSET_ELT_WORDS; i++)
 {
+  bitset_word word = srcp[i];
+  if (word)
+BITSET_FOR_EACH_BIT (pos, word)
+  {
+list[count++] = bitno + pos;
+if (count >= num)
+  {
+*next = bitno + pos + 1;
+return count;
+  }
+  }
+  wind

[PATCH 0/6] bitset: more conversions to ffs

[Akim Demaille]

With this, I believe I have used ffs everywhere possible.  When gnulib
have fls* support, we will be able do the same for
bitset_list_reverse, but I'm not sure it's a frequent need.

FWIW, I have run successfully the test suite of Bison on every single
bitset implementation.

Cheers!

Akim Demaille (6):
  bitset: be sure to always return a value
  bitset: check empty and full bitsets
  bitset: use ffs where possible in the table implementation
  bitset: use ffs where possible in the vector implementation
  bitset: tests: try harder to break it
  bitset: tests: exercise the stats too

 ChangeLog   |  39 +
 lib/bitset/array.c  |  10 ++---
 lib/bitset/stats.c  |   8 +++-
 lib/bitset/table.c  | 104 +++-
 lib/bitset/vector.c |  57 ++--
 tests/test-bitset.c |  88 -
 6 files changed, 176 insertions(+), 130 deletions(-)

-- 
2.29.2

[PATCH 1/6] bitset: be sure to always return a value

[Akim Demaille]

* lib/bitset/array.c (abitset_small_list): Always update *next and
return a value.
---
 ChangeLog  |  6 ++
 lib/bitset/array.c | 10 --
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index d50f4d1a2..88ae15003 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2020-11-19  Akim Demaille  
+
+   bitset: be sure to always return a value
+   * lib/bitset/array.c (abitset_small_list): Always update *next and
+   return a value.
+
 2020-11-17  Akim Demaille  
 
bitset: strengthen tests
diff --git a/lib/bitset/array.c b/lib/bitset/array.c
index 3f8bcca87..0e90b6b1f 100644
--- a/lib/bitset/array.c
+++ b/lib/bitset/array.c
@@ -65,12 +65,8 @@ abitset_small_list (bitset src, bitset_bindex *list,
   bitset_bindex count = 0;
   /* Is there enough room to avoid checking in each iteration? */
   if (num >= BITSET_WORD_BITS)
-{
-  BITSET_FOR_EACH_BIT (pos, word)
-list[count++] = bitno + pos;
-  *next = bitno + BITSET_WORD_BITS;
-  return count;
-}
+BITSET_FOR_EACH_BIT (pos, word)
+  list[count++] = bitno + pos;
   else
 BITSET_FOR_EACH_BIT (pos, word)
   {
@@ -81,6 +77,8 @@ abitset_small_list (bitset src, bitset_bindex *list,
 return count;
   }
   }
+  *next = bitno + BITSET_WORD_BITS;
+  return count;
 }
 
 
-- 
2.29.2

Re: bitpack search in bitsets

[Akim Demaille]

Bruno,

> Le 17 nov. 2020 à 22:50, Bruno Haible  a écrit :
> 
> Hi Akim,
> 
> In gnulib/lib/ssfmalloc-bitmap.h there is a function
> 
> /* Returns the smallest index k >= 0 for which the bit packet of c consecutive
>   bits (1 <= c <= 32) is all set in the bitmap consisting of num_words words.
>   Returns (size_t)(-1) if there is none.  */
> 
> I can easily extend this to c > 32. So, this will be a one-pass search for
> c consecutive bits in a bitmap, that operates on words and fetches every word
> only twice at most.
> 
> Is this something that could be useful for your 'bitset' module?

(FTR, I am not the author, Michael Hayes is.  It was a contribution
for GCC, that actually didn't make it into GCC.  But I liked it very much
and took it for Bison.)

That doesn't ring any bell.  I don't remember I saw any feature in bitset
that depends on consecutive set bits, and a quick search did not find
anything.

Conversely: couldn't ssfmalloc sit on top of bitset?

Re: [PATCH 0/6] bitset: clean up, test, fix, accelerate

[Akim Demaille]

Hi Bruno,

> Le 17 nov. 2020 à 22:42, Bruno Haible  a écrit :
> 
> Hi Akim,
> 
>> The last commit fixes an error caught by the
>> "strengthen tests" commit.  I can easily commute them if we want to
>> avoid "make check" failures in the history.
> 
> Yes, please. Some people use 'git bisect'; therefore it's useful if
> there are no easily-avoidable "make check" failures in the history.

That's also the policy in Bison, for the same reasons.

Commuted and pushed.

Cheers!

Re: [PATCH 5/5] bitset: use ffsl to accelerate iterations over set bits

[Akim Demaille]

Hi Bruno,

> Le 18 nov. 2020 à 04:40, Bruno Haible  a écrit :
> 
> Hi Akim,
> 
>>> Yes, assuming you did a quick check with the unit tests:
>>> ./gnulib-tool --test --single-configure bitset
>> 
>> Yes, I did run the test suite :)
>> 
>> Installed.  Thanks!
> 
> A testdir of all of gnulib now shows 1 test failure:

tbitset{n_bits = 229, set = {0 1 }}
../../gltests/test-bitset.c:171: assertion 'first == bitset_first (bdst)' failed

> This is on a glibc system, x86_64, with gcc 5.4.0.
> 
> Is this bug the one that you are fixing in the new patch series, or a 
> different
> one?

Most likely the same one: bitset_first uses bitset_list under the
hood, and it's tbitset_list that I fixed in the other series.

It turns out that on my machine it was not revealed, because when it comes
to tbitset, the random size that was picked is small enough not to fall
into the trap.  That's why I added deterministic tests for "large" bit sets.

[PATCH 0/6] bitset: clean up, test, fix, accelerate

[Akim Demaille]

This is the continuation of the work prompted by Bruno's remark that
bitset should use ffs
(https://lists.gnu.org/archive/html/bug-gnulib/2020-11/msg00052.html).

I'm not fully done yet, but here is another batch of
improvements/fixes.  The last commit fixes an error caught by the
"strengthen tests" commit.  I can easily commute them if we want to
avoid "make check" failures in the history.

Cheers!

Akim Demaille (6):
  bitset: use ffs where possible in array implementation
  bitset: use ffs where possible in the list implementation
  bitset: test: run deterministic tests on several bitset sizes
  bitset: strengthen tests
  bitset: rename internal details for consistency
  bitset: fix iteration over table bitsets

 ChangeLog   |  34 +
 lib/bitset/array.c  |  46 ++-
 lib/bitset/list.c   | 100 --
 lib/bitset/table.c  | 323 ++--
 tests/test-bitset.c | 186 +
 5 files changed, 331 insertions(+), 358 deletions(-)

-- 
2.29.2

[PATCH 6/6] bitset: fix iteration over table bitsets

[Akim Demaille]

* lib/bitset/table.c (tbitset_list): Update bitno when windex is.
---
 ChangeLog  | 5 +
 lib/bitset/table.c | 5 +++--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index f3df478d1..1aa7317a9 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2020-11-17  Akim Demaille  
+
+   bitset: fix iteration over table bitsets
+   * lib/bitset/table.c (tbitset_list): Update bitno when windex is.
+
 2020-11-17  Akim Demaille  
 
bitset: rename internal details for consistency
diff --git a/lib/bitset/table.c b/lib/bitset/table.c
index b762f5951..d111e0203 100644
--- a/lib/bitset/table.c
+++ b/lib/bitset/table.c
@@ -649,15 +649,16 @@ tbitset_list (bitset bset, bitset_bindex *list,
 
   for (; eindex < size; eindex++)
 {
-  bitset_word *srcp;
 
   tbitset_elt *elt = elts[eindex];
   if (!elt)
 continue;
 
-  srcp = TBITSET_WORDS (elt);
+  bitset_word *srcp = TBITSET_WORDS (elt);
   bitset_windex windex = eindex * TBITSET_ELT_WORDS;
+  bitno = windex * BITSET_WORD_BITS;
 
+  /* Is there enough room to avoid checking in each iteration? */
   if ((count + TBITSET_ELT_BITS) < num)
 {
   /* The coast is clear, plant boot!  */
-- 
2.29.2

[PATCH 5/6] bitset: rename internal details for consistency

[Akim Demaille]

The "table" implementation used to called "expandable" (see
https://lists.gnu.org/archive/html/bug-gnulib/2018-11/msg00096.html).
Clean up remaining traces of "expandable".

* lib/bitset/table.c: Rename all the EBITSET_ symbols as TBITSET_.
---
 ChangeLog  |   5 +
 lib/bitset/table.c | 320 ++---
 2 files changed, 165 insertions(+), 160 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index ad9c9e153..f3df478d1 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2020-11-17  Akim Demaille  
+
+   bitset: rename internal details for consistency
+   * lib/bitset/table.c: Rename all the EBITSET_ symbols as TBITSET_.
+
 2020-11-17  Akim Demaille  
 
bitset: strengthen tests
diff --git a/lib/bitset/table.c b/lib/bitset/table.c
index c80eebffa..b762f5951 100644
--- a/lib/bitset/table.c
+++ b/lib/bitset/table.c
@@ -49,18 +49,18 @@
 
 
 /* Number of words to use for each element.  */
-#define EBITSET_ELT_WORDS 2
+#define TBITSET_ELT_WORDS 2
 
 /* Number of bits stored in each element.  */
-#define EBITSET_ELT_BITS \
-  ((unsigned) (EBITSET_ELT_WORDS * BITSET_WORD_BITS))
+#define TBITSET_ELT_BITS \
+  ((unsigned) (TBITSET_ELT_WORDS * BITSET_WORD_BITS))
 
-/* Ebitset element.  We use an array of bits.  */
+/* Tbitset element.  We use an array of bits.  */
 typedef struct tbitset_elt_struct
 {
   union
   {
-bitset_word words[EBITSET_ELT_WORDS];   /* Bits that are set.  */
+bitset_word words[TBITSET_ELT_WORDS];   /* Bits that are set.  */
 struct tbitset_elt_struct *next;
   }
   u;
@@ -73,13 +73,13 @@ typedef tbitset_elt *tbitset_elts;
 
 /* Number of elements to initially allocate.  */
 
-#ifndef EBITSET_INITIAL_SIZE
-# define EBITSET_INITIAL_SIZE 2
+#ifndef TBITSET_INITIAL_SIZE
+# define TBITSET_INITIAL_SIZE 2
 #endif
 
 
 enum tbitset_find_mode
-  { EBITSET_FIND, EBITSET_CREATE, EBITSET_SUBST };
+  { TBITSET_FIND, TBITSET_CREATE, TBITSET_SUBST };
 
 static tbitset_elt tbitset_zero_elts[1]; /* Elements of all zero bits.  */
 
@@ -88,33 +88,33 @@ static struct obstack tbitset_obstack;
 static bool tbitset_obstack_init = false;
 static tbitset_elt *tbitset_free_list;  /* Free list of bitset elements.  */
 
-#define EBITSET_N_ELTS(N) (((N) + EBITSET_ELT_BITS - 1) / EBITSET_ELT_BITS)
-#define EBITSET_ELTS(BSET) ((BSET)->e.elts)
-#define EBITSET_SIZE(BSET) EBITSET_N_ELTS (BITSET_NBITS_ (BSET))
-#define EBITSET_ASIZE(BSET) ((BSET)->e.size)
+#define TBITSET_N_ELTS(N) (((N) + TBITSET_ELT_BITS - 1) / TBITSET_ELT_BITS)
+#define TBITSET_ELTS(BSET) ((BSET)->e.elts)
+#define TBITSET_SIZE(BSET) TBITSET_N_ELTS (BITSET_NBITS_ (BSET))
+#define TBITSET_ASIZE(BSET) ((BSET)->e.size)
 
-#define EBITSET_NEXT(ELT) ((ELT)->u.next)
-#define EBITSET_WORDS(ELT) ((ELT)->u.words)
+#define TBITSET_NEXT(ELT) ((ELT)->u.next)
+#define TBITSET_WORDS(ELT) ((ELT)->u.words)
 
 /* Disable bitset cache and mark BSET as being zero.  */
-#define EBITSET_ZERO_SET(BSET) ((BSET)->b.cindex = BITSET_WINDEX_MAX, \
+#define TBITSET_ZERO_SET(BSET) ((BSET)->b.cindex = BITSET_WINDEX_MAX, \
 (BSET)->b.cdata = 0)
 
-#define EBITSET_CACHE_DISABLE(BSET)  ((BSET)->b.cindex = BITSET_WINDEX_MAX)
+#define TBITSET_CACHE_DISABLE(BSET)  ((BSET)->b.cindex = BITSET_WINDEX_MAX)
 
 /* Disable bitset cache and mark BSET as being possibly non-zero.  */
-#define EBITSET_NONZERO_SET(BSET) \
- (EBITSET_CACHE_DISABLE (BSET), (BSET)->b.cdata = (bitset_word *)~0)
+#define TBITSET_NONZERO_SET(BSET) \
+ (TBITSET_CACHE_DISABLE (BSET), (BSET)->b.cdata = (bitset_word *)~0)
 
 /* A conservative estimate of whether the bitset is zero.
This is non-zero only if we know for sure that the bitset is zero.  */
-#define EBITSET_ZERO_P(BSET) ((BSET)->b.cdata == 0)
+#define TBITSET_ZERO_P(BSET) ((BSET)->b.cdata == 0)
 
 /* Enable cache to point to element with table index EINDEX.
The element must exist.  */
-#define EBITSET_CACHE_SET(BSET, EINDEX) \
- ((BSET)->b.cindex = (EINDEX) * EBITSET_ELT_WORDS, \
-  (BSET)->b.cdata = EBITSET_WORDS (EBITSET_ELTS (BSET) [EINDEX]))
+#define TBITSET_CACHE_SET(BSET, EINDEX) \
+ ((BSET)->b.cindex = (EINDEX) * TBITSET_ELT_WORDS, \
+  (BSET)->b.cdata = TBITSET_WORDS (TBITSET_ELTS (BSET) [EINDEX]))
 
 #undef min
 #undef max
@@ -127,14 +127,14 @@ tbitset_resize (bitset src, bitset_bindex n_bits)
   if (n_bits == BITSET_NBITS_ (src))
 return n_bits;
 
-  bitset_windex oldsize = EBITSET_SIZE (src);
-  bitset_windex newsize = EBITSET_N_ELTS (n_bits);
+  bitset_windex oldsize = TBITSET_SIZE (src);
+  bitset_windex newsize = TBITSET_N_ELTS (n_bits);
 
   if (oldsize < newsize)
 {
   /* The bitset needs to grow.  If we already have enough memory
  allocated, then just zero what we need.  */
-  if (newsize > EBITSET_ASIZE (src))
+  if (newsize > TBITSET_ASIZE (src))
 {
   /* We need to allocate more memory.  When oldsize is
  

[PATCH 4/6] bitset: strengthen tests

[Akim Demaille]

* tests/test-bitset.c (compare): Also check count.
Deal only with random values, move the one-bit tests to...
(check_one_bit): this new function.
(check_attributes): Call it.
---
 ChangeLog   |   8 +++
 tests/test-bitset.c | 165 +++-
 2 files changed, 93 insertions(+), 80 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 82b67f481..ad9c9e153 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@
+2020-11-17  Akim Demaille  
+
+   bitset: strengthen tests
+   * tests/test-bitset.c (compare): Also check count.
+   Deal only with random values, move the one-bit tests to...
+   (check_one_bit): this new function.
+   (check_attributes): Call it.
+
 2020-11-17  Akim Demaille  
 
bitset: test: run deterministic tests on several bitset sizes
diff --git a/tests/test-bitset.c b/tests/test-bitset.c
index 9c79d04ff..5068ae782 100644
--- a/tests/test-bitset.c
+++ b/tests/test-bitset.c
@@ -72,6 +72,9 @@ void compare (enum bitset_attr a, enum bitset_attr b)
   bitset adst = bitset_create (nbits, a);
   bitset bdst = bitset_create (nbits, b);
 
+  /* count */
+  ASSERT (bitset_count (asrc0) == bitset_count (bsrc0));
+
   /* not */
   bitset_not (adst, asrc0);
   bitset_not (bdst, bsrc0);
@@ -143,88 +146,57 @@ void compare (enum bitset_attr a, enum bitset_attr b)
   bitset_zero (bdst);
   assert_bitset_equal (adst, bdst);
 
-  /* first and last and FOR_EACH.
-
- Exercise on random values (i == -1), but also on all the
- single-bit values: it's easy to get the handling of the most
- significant bit wrong.  */
-  for (int i = -1; i < nbits; ++i)
-{
-  /* Work on bdst to exercise all the bitset types (adst is
- BITSET_VARIABLE).  */
-  if (i >= 0)
-{
-  bitset_zero (bdst);
-  bitset_set (bdst, i);
-}
-  else
-{
-  bitset_copy (bdst, bsrc0);
-  debug_bitset (bdst);
-  debug_bitset (bsrc0);
-}
-  bitset_copy (adst, bdst);
-
-  /* first and last */
+  /* first and last and FOR_EACH.  */
+  /* Work on bdst to exercise all the bitset types (adst is
+ BITSET_VARIABLE).  */
+  bitset_copy (bdst, bsrc0);
+  debug_bitset (bdst);
+  debug_bitset (bsrc0);
+  bitset_copy (adst, bdst);
+
+  /* count. */
+  ASSERT (bitset_count (adst) == bitset_count (bdst));
+
+  /* first and last */
+  {
+bitset_bindex first = bitset_first (adst);
+ASSERT (first == bitset_first (bdst));
+
+bitset_bindex last  = bitset_last (adst);
+ASSERT (last == bitset_last (bdst));
+
+ASSERT (first <= last);
+  }
+
+
+  /* FOR_EACH.  */
+  {
+bitset_iterator iter;
+bitset_bindex j;
+bitset_bindex first = bitset_first (bdst);
+bitset_bindex last  = bitset_last (bdst);
+bool seen_first = false;
+bool seen_last = false;
+BITSET_FOR_EACH (iter, bdst, j, 0)
   {
-bitset_bindex first = bitset_first (adst);
-ASSERT (first == bitset_first (bdst));
-
-bitset_bindex last  = bitset_last (adst);
-ASSERT (last == bitset_last (bdst));
-
-if (i >= 0)
-  {
-ASSERT (first == i);
-ASSERT (last == i);
-  }
-
-if (first != BITSET_BINDEX_MAX)
-  {
-ASSERT (last != BITSET_BINDEX_MAX);
-ASSERT (first <= last);
-ASSERT (bitset_test (adst, first));
-ASSERT (bitset_test (adst, last));
-ASSERT (bitset_test (bdst, first));
-ASSERT (bitset_test (bdst, last));
-  }
-else
-  ASSERT (last == BITSET_BINDEX_MAX);
+ASSERT (first <= j && j <= last);
+ASSERT (bitset_test (bdst, j));
+if (j == first)
+  seen_first = true;
+if (j == last)
+  seen_last = true;
   }
-
-
-  /* FOR_EACH.  */
+if (first == BITSET_BINDEX_MAX)
   {
-bitset_iterator iter;
-bitset_bindex j;
-bitset_bindex first = bitset_first (bdst);
-bitset_bindex last  = bitset_last (bdst);
-bool seen_first = false;
-bool seen_last = false;
-BITSET_FOR_EACH (iter, bdst, j, 0)
-  {
-ASSERT (first <= j && j <= last);
-ASSERT (bitset_test (bdst, j));
-if (j == first)
-  seen_first = true;
-if (j == last)
-  seen_last = true;
-if (0 <= i)
-  ASSERT (j == i);
-  }
-if (first == BITSET_BINDEX_MAX)
-  {
-ASSERT (!seen_first);
-ASSERT (!seen_last);
-  }
-else
-  {
-ASSERT (seen_first);
-ASSERT (seen_last);
-  }
+ASSERT (!seen_first);
+ASSERT (!seen_last);
   }
-}
-
+else
+  {
+ASSERT (seen_first);
+ASSERT (seen_last);
+  }
+  }
 
   /* resize.
 
@@ -252,11 +224,39 @@ void compare (enum bitset_a

[PATCH 3/6] bitset: test: run deterministic tests on several bitset sizes

[Akim Demaille]

* tests/test-bitset.c (check_attributes): Run it with small and large
sizes.
---
 ChangeLog   |  6 ++
 tests/test-bitset.c | 23 +--
 2 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 1b3bcbc32..82b67f481 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2020-11-17  Akim Demaille  
+
+   bitset: test: run deterministic tests on several bitset sizes
+   * tests/test-bitset.c (check_attributes): Run it with small and large
+   sizes.
+
 2020-11-17  Akim Demaille  
 
bitset: use ffs where possible in the list implementation
diff --git a/tests/test-bitset.c b/tests/test-bitset.c
index ebe9e1d55..9c79d04ff 100644
--- a/tests/test-bitset.c
+++ b/tests/test-bitset.c
@@ -256,12 +256,10 @@ void compare (enum bitset_attr a, enum bitset_attr b)
having attributes ATTR.  */
 
 static
-void check_attributes (enum bitset_attr attr)
+void check_attributes (enum bitset_attr attr, int nbits)
 {
-  enum { nbits = 32 };
-
   bitset bs0 = bitset_create (nbits, attr);
-  ASSERT (bitset_size (bs0) == 32);
+  ASSERT (bitset_size (bs0) == nbits);
   ASSERT (bitset_count (bs0) == 0);
   ASSERT (bitset_empty_p (bs0));
 
@@ -297,12 +295,17 @@ void check_attributes (enum bitset_attr attr)
 
 int main (void)
 {
-  check_attributes (BITSET_FIXED);
-  check_attributes (BITSET_VARIABLE);
-  check_attributes (BITSET_DENSE);
-  check_attributes (BITSET_SPARSE);
-  check_attributes (BITSET_FRUGAL);
-  check_attributes (BITSET_GREEDY);
+  for (int i = 0; i < 2; ++i)
+{
+  /* table bitsets have elements that store 256 bits.  */
+  int nbits = i == 0 ? 32 : 257;
+  check_attributes (BITSET_FIXED,nbits);
+  check_attributes (BITSET_VARIABLE, nbits);
+  check_attributes (BITSET_DENSE,nbits);
+  check_attributes (BITSET_SPARSE,   nbits);
+  check_attributes (BITSET_FRUGAL,   nbits);
+  check_attributes (BITSET_GREEDY,   nbits);
+}
 
   compare (BITSET_VARIABLE, BITSET_FIXED);
   compare (BITSET_VARIABLE, BITSET_VARIABLE);
-- 
2.29.2

[PATCH 2/6] bitset: use ffs where possible in the list implementation

[Akim Demaille]

* lib/bitset/list.c (lbitset_list): Use BITSET_FOR_EACH_BIT.
---
 ChangeLog |   5 +++
 lib/bitset/list.c | 100 +++---
 2 files changed, 28 insertions(+), 77 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index ba738c8c5..1b3bcbc32 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2020-11-17  Akim Demaille  
+
+   bitset: use ffs where possible in the list implementation
+   * lib/bitset/list.c (lbitset_list): Use BITSET_FOR_EACH_BIT.
+
 2020-11-17  Akim Demaille  
 
bitset: use ffs where possible in array implementation
diff --git a/lib/bitset/list.c b/lib/bitset/list.c
index dc00fdc29..96f163087 100644
--- a/lib/bitset/list.c
+++ b/lib/bitset/list.c
@@ -691,19 +691,14 @@ lbitset_list (bitset bset, bitset_bindex *list,
   for (; (windex - elt->index) < LBITSET_ELT_WORDS; windex++)
 {
   bitset_word word = srcp[windex - elt->index] >> (bitno % 
BITSET_WORD_BITS);
-
-  for (; word; bitno++)
+  BITSET_FOR_EACH_BIT (pos, word)
 {
-  if (word & 1)
+  list[count++] = bitno + pos;
+  if (count >= num)
 {
-  list[count++] = bitno;
-  if (count >= num)
-{
-  *next = bitno + 1;
-  return count;
-}
+  *next = bitno + pos + 1;
+  return count;
 }
-  word >>= 1;
 }
   bitno = (windex + 1) * BITSET_WORD_BITS;
 }
@@ -717,14 +712,11 @@ lbitset_list (bitset bset, bitset_bindex *list,
 }
 }
 
-
-  /* If num is 1, we could speed things up with a binary search
- of the word of interest.  */
-
   while (elt)
 {
   bitset_word *srcp = elt->words;
 
+  /* Is there enough room to avoid checking in each iteration? */
   if ((count + LBITSET_ELT_BITS) < num)
 {
   /* The coast is clear, plant boot!  */
@@ -732,42 +724,15 @@ lbitset_list (bitset bset, bitset_bindex *list,
 #if LBITSET_ELT_WORDS == 2
   bitset_word word = srcp[0];
   if (word)
-{
-  if (!(word & 0x))
-{
-  word >>= 16;
-  bitno += 16;
-}
-  if (!(word & 0xff))
-{
-  word >>= 8;
-  bitno += 8;
-}
-  for (; word; bitno++)
-{
-  if (word & 1)
-list[count++] = bitno;
-  word >>= 1;
-}
-}
+BITSET_FOR_EACH_BIT (pos, word)
+  list[count++] = bitno + pos;
   windex++;
   bitno = windex * BITSET_WORD_BITS;
 
   word = srcp[1];
   if (word)
-{
-  if (!(word & 0x))
-{
-  word >>= 16;
-  bitno += 16;
-}
-  for (; word; bitno++)
-{
-  if (word & 1)
-list[count++] = bitno;
-  word >>= 1;
-}
-}
+BITSET_FOR_EACH_BIT (pos, word)
+  list[count++] = bitno + pos;
   windex++;
   bitno = windex * BITSET_WORD_BITS;
 #else
@@ -775,24 +740,8 @@ lbitset_list (bitset bset, bitset_bindex *list,
 {
   bitset_word word = srcp[i];
   if (word)
-{
-  if (!(word & 0x))
-{
-  word >>= 16;
-  bitno += 16;
-}
-  if (!(word & 0xff))
-{
-  word >>= 8;
-  bitno += 8;
-}
-  for (; word; bitno++)
-{
-  if (word & 1)
-list[count++] = bitno;
-  word >>= 1;
-}
-}
+BITSET_FOR_EACH_BIT (pos, word)
+  list[count++] = bitno + pos;
   windex++;
   bitno = windex * BITSET_WORD_BITS;
 }
@@ -802,22 +751,19 @@ lbitset_list (bitset bset, bitset_bindex *list,
 {
   /* Tread more carefully since we need to check
  if array overflows.  */
-
   for (int i = 0; i < LBITSET_ELT_WORDS; i++)
 {
-  for (bitset_word word = srcp[i]; word; bitno++)
-{
-  if (word & 1)
-{
-  list[count++] = bitno;
-  if (co

[PATCH 1/6] bitset: use ffs where possible in array implementation

[Akim Demaille]

* lib/bitset/array.c (abitset_small_list): Use BITSET_FOR_EACH_BIT.
---
 ChangeLog  |  5 +
 lib/bitset/array.c | 46 +++---
 2 files changed, 20 insertions(+), 31 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index c8f4ffaac..ba738c8c5 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2020-11-17  Akim Demaille  
+
+   bitset: use ffs where possible in array implementation
+   * lib/bitset/array.c (abitset_small_list): Use BITSET_FOR_EACH_BIT.
+
 2020-11-16  Bruno Haible  
 
Fix link errors on platforms with libunistring.
diff --git a/lib/bitset/array.c b/lib/bitset/array.c
index 6c5f7ed34..3f8bcca87 100644
--- a/lib/bitset/array.c
+++ b/lib/bitset/array.c
@@ -62,38 +62,25 @@ abitset_small_list (bitset src, bitset_bindex *list,
 
   word >>= bitno;
 
-  /* If num is 1, we could speed things up with a binary search
- of the word of interest.  */
-
-  bitset_bindex count;
+  bitset_bindex count = 0;
+  /* Is there enough room to avoid checking in each iteration? */
   if (num >= BITSET_WORD_BITS)
 {
-  for (count = 0; word; bitno++)
-{
-  if (word & 1)
-list[count++] = bitno;
-  word >>= 1;
-}
+  BITSET_FOR_EACH_BIT (pos, word)
+list[count++] = bitno + pos;
+  *next = bitno + BITSET_WORD_BITS;
+  return count;
 }
   else
-{
-  for (count = 0; word; bitno++)
-{
-  if (word & 1)
-{
-  list[count++] = bitno;
-  if (count >= num)
-{
-  bitno++;
-  break;
-}
-}
-  word >>= 1;
-}
-}
-
-  *next = bitno;
-  return count;
+BITSET_FOR_EACH_BIT (pos, word)
+  {
+list[count++] = bitno + pos;
+if (count >= num)
+  {
+*next = bitno + pos + 1;
+return count;
+  }
+  }
 }
 
 
@@ -205,9 +192,6 @@ abitset_list (bitset src, bitset_bindex *list,
   if (windex >= size)
 return 0;
 
-  /* If num is 1, we could speed things up with a binary search
- of the current word.  */
-
   bitoff = windex * BITSET_WORD_BITS;
 }
   else
-- 
2.29.2

Re: [PATCH 5/5] bitset: use ffsl to accelerate iterations over set bits

[Akim Demaille]

> Le 15 nov. 2020 à 19:07, Bruno Haible  a écrit :
> 
> Hi Akim,
> 
>> Thanks for catching these errors.  I have fixed them.
>> Good to install?
> 
> Yes, assuming you did a quick check with the unit tests:
>  ./gnulib-tool --test --single-configure bitset

Yes, I did run the test suite :)

Installed.  Thanks!

Re: [PATCH 5/5] bitset: use ffsl to accelerate iterations over set bits

[Akim Demaille]

Hi Bruno,

> Le 15 nov. 2020 à 17:42, Bruno Haible  a écrit :
> 
> Akim Demaille wrote:
>> @@ -20,6 +20,7 @@ Depends-on:
>> attribute
>> c99
>> fopen-gnu
>> +fssl
> 
> Typo: We don't have a module named 'fssl'.

Thanks for catching these errors.  I have fixed them.
Good to install?

[PATCH 5/5] bitset: use ffsl to accelerate iterations over set bits

[Akim Demaille]

Currently we iterate over words bit by bit.  Instead, we should jump
from set bit to set bit.
Suggested by Bruno Haible.

* modules/bitset: Depend upon ffsl.
* lib/bitset/base.h (bitset_ffs, BITSET_FOR_EACH_BIT): New.
* lib/bitset/array.c (abitset_list): Use BITSET_FOR_EACH_BIT.
---
 ChangeLog  |  6 ++
 lib/bitset/array.c | 50 +-
 lib/bitset/base.h  | 17 
 modules/bitset |  1 +
 4 files changed, 42 insertions(+), 32 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 713206bcf..d2f104af3 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,11 @@
 2020-11-15  Akim Demaille  
 
+   bitset: use ffsl to accelerate iterations over set bits
+   Suggested by Bruno Haible.
+   * modules/bitset: Depend upon ffsl.
+   * lib/bitset/base.h (bitset_ffs, BITSET_FOR_EACH_BIT): New.
+   * lib/bitset/array.c (abitset_list): Use BITSET_FOR_EACH_BIT.
+
bitset: more tests
* tests/test-bitset.c (compare): Make it clear that the random values
should not be modified.
diff --git a/lib/bitset/array.c b/lib/bitset/array.c
index 1db583891..6c5f7ed34 100644
--- a/lib/bitset/array.c
+++ b/lib/bitset/array.c
@@ -227,18 +227,15 @@ abitset_list (bitset src, bitset_bindex *list,
 
   bitoff = windex * BITSET_WORD_BITS;
   bitset_word word = srcp[windex] >> bitno;
-  for (bitno = bitoff + bitno; word; bitno++)
+  bitno = bitoff + bitno;
+  BITSET_FOR_EACH_BIT (pos, word)
 {
-  if (word & 1)
+  list[count++] = bitno + pos;
+  if (count >= num)
 {
-  list[count++] = bitno;
-  if (count >= num)
-{
-  *next = bitno + 1;
-  return count;
-}
+  *next = bitno + pos + 1;
+  return count;
 }
-  word >>= 1;
 }
   windex++;
 }
@@ -251,31 +248,20 @@ abitset_list (bitset src, bitset_bindex *list,
   if (!word)
 continue;
 
+  /* Is there enough room to avoid checking in each iteration? */
   if ((count + BITSET_WORD_BITS) < num)
-{
-  for (bitno = bitoff; word; bitno++)
-{
-  if (word & 1)
-list[count++] = bitno;
-  word >>= 1;
-}
-}
+BITSET_FOR_EACH_BIT (pos, word)
+  list[count++] = bitoff + pos;
   else
-{
-  for (bitno = bitoff; word; bitno++)
-{
-  if (word & 1)
-{
-  list[count++] = bitno;
-  if (count >= num)
-{
-  *next = bitno + 1;
-  return count;
-}
-}
-  word >>= 1;
-}
-}
+BITSET_FOR_EACH_BIT (pos, word)
+  {
+list[count++] = bitoff + pos;
+if (count >= num)
+  {
+*next = bitoff + pos + 1;
+return count;
+  }
+  }
 }
 
   *next = bitoff;
diff --git a/lib/bitset/base.h b/lib/bitset/base.h
index 2ae7b2080..50569a0fc 100644
--- a/lib/bitset/base.h
+++ b/lib/bitset/base.h
@@ -24,6 +24,7 @@
 #include 
 #include 
 #include 
+#include  /* ffsl */
 
 #include "attribute.h"
 #include "xalloc.h"
@@ -52,6 +53,14 @@ extern const char * const bitset_type_names[];
 typedef unsigned long bitset_word;
 #define BITSET_WORD_BITS ((unsigned) (CHAR_BIT * sizeof (bitset_word)))
 
+/* Iterate over each set bit of WORD.
+   Each iteration sets POS to the 0-based index of the next set bit in WORD.
+   Repeatedly resets bits in WORD in place until it's null.  */
+#define BITSET_FOR_EACH_BIT(Pos, Word)  \
+  for (int Pos = bitset_ffs (Word); \
+   0 <= Pos;\
+   Word ^= 1UL << Pos, Pos = bitset_ffs (Word))
+
 /* Bit index.  In theory we might need a type wider than size_t, but
in practice we lose at most a factor of CHAR_BIT by going with
size_t, and that is good enough.  If this type is changed to be
@@ -60,6 +69,14 @@ typedef unsigned long bitset_word;
The bit and word index types must be unsigned.  */
 typedef size_t bitset_bindex;
 
+/* First first set bit in WORD.
+   Indexes start at 0, return -1 if WORD is null. */
+static inline
+int bitset_ffs (bitset_word word)
+{
+  return ffsl ((long) word) - 1;
+}
+
 /* Word index.  */
 typedef size_t bitset_windex;
 
diff --git a/modules/bitset b/modules/bitset
index 21cde24ac..c65471a02 100644
--- a/modules/bitset
+++ b/modules/bitset
@@ -20,6 +20,7 @@ Depends-on:
 attribute
 c99
 fopen-gnu
+fssl
 gettext-h
 obstack
 xalloc
-- 
2.29.2

[PATCH 3/5] bitset: fix the copy from lbitset to other types

[Akim Demaille]

bitset_copy from an lbitset did not check whether the destination has
the same type.  Apply the same strategy as elsewhere.

Without this commit, the following one fails.

* lib/bitset/list.c (lbitset_copy): Rename as...
(lbitset_copy_): this.
(lbitset_copy): New.
Dispatch to heterogeneous/homogeneous copy.
---
 ChangeLog |  6 ++
 lib/bitset/list.c | 11 ++-
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/ChangeLog b/ChangeLog
index 3e3e78ed3..ff21b6631 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,11 @@
 2020-11-15  Akim Demaille  
 
+   bitset: fix the copy from lbitset to other types
+   * lib/bitset/list.c (lbitset_copy): Rename as...
+   (lbitset_copy_): this.
+   (lbitset_copy): New.
+   Dispatch to heterogeneous/homogeneous copy.
+
bitset: making debug traces more useful
* lib/bitset.c (bitset_print): Print the bitset type in verbose node.
 
diff --git a/lib/bitset/list.c b/lib/bitset/list.c
index c1f3d9b15..dc00fdc29 100644
--- a/lib/bitset/list.c
+++ b/lib/bitset/list.c
@@ -428,7 +428,7 @@ lbitset_equal_p (bitset dst, bitset src)
 
 /* Copy bits from bitset SRC to bitset DST.  */
 static inline void
-lbitset_copy (bitset dst, bitset src)
+lbitset_copy_ (bitset dst, bitset src)
 {
   if (src == dst)
 return;
@@ -463,6 +463,15 @@ lbitset_copy (bitset dst, bitset src)
 }
 
 
+static void
+lbitset_copy (bitset dst, bitset src)
+{
+  if (BITSET_COMPATIBLE_ (dst, src))
+lbitset_copy_ (dst, src);
+  else
+bitset_copy_ (dst, src);
+}
+
 /* Copy bits from bitset SRC to bitset DST.  Return true if
bitsets different.  */
 static inline bool
-- 
2.29.2

[PATCH 2/5] bitset: making debug traces more useful

[Akim Demaille]

* lib/bitset.c (bitset_print): Print the bitset type in verbose node.
---
 ChangeLog| 3 +++
 lib/bitset.c | 5 +++--
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index f25a948eb..3e3e78ed3 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,8 @@
 2020-11-15  Akim Demaille  
 
+   bitset: making debug traces more useful
+   * lib/bitset.c (bitset_print): Print the bitset type in verbose node.
+
bitset: comment changes
* lib/bitset.c: Move some documenting comments to...
* lib/bitset.h: here.
diff --git a/lib/bitset.c b/lib/bitset.c
index 06a47ad64..26d176e18 100644
--- a/lib/bitset.c
+++ b/lib/bitset.c
@@ -271,7 +271,8 @@ static void
 bitset_print (FILE *file, bitset bset, bool verbose)
 {
   if (verbose)
-fprintf (file, "n_bits = %lu, set = {",
+fprintf (file, "%s{n_bits = %lu, set = {",
+ bitset_type_name_get (bset),
  (unsigned long) bitset_size (bset));
 
   unsigned pos = 30;
@@ -290,7 +291,7 @@ bitset_print (FILE *file, bitset bset, bool verbose)
   }
 
   if (verbose)
-fprintf (file, "}\n");
+fprintf (file, "}}\n");
 }
 
 
-- 
2.29.2

[PATCH 1/5] bitset: comment changes

[Akim Demaille]

* lib/bitset.c: Move some documenting comments to...
* lib/bitset.h: here.
* lib/bitset/array.c: Fix some comments.
---
 ChangeLog  |  7 +++
 lib/bitset.c   |  4 
 lib/bitset.h   | 20 
 lib/bitset/array.c |  6 +++---
 4 files changed, 22 insertions(+), 15 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 4abbcd385..f25a948eb 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,10 @@
+2020-11-15  Akim Demaille  
+
+   bitset: comment changes
+   * lib/bitset.c: Move some documenting comments to...
+   * lib/bitset.h: here.
+   * lib/bitset/array.c: Fix some comments.
+
 2020-11-14  Paul Eggert  
 
careadlinkat: warn better about GCC bug 93644
diff --git a/lib/bitset.c b/lib/bitset.c
index 70752b621..06a47ad64 100644
--- a/lib/bitset.c
+++ b/lib/bitset.c
@@ -207,8 +207,6 @@ bitset_type_name_get (bitset bset)
 }
 
 
-/* Find next bit set in SRC starting from and including BITNO.
-   Return BITSET_BINDEX_MAX if SRC empty.  */
 bitset_bindex
 bitset_next (bitset src, bitset_bindex bitno)
 {
@@ -228,8 +226,6 @@ bitset_compatible_p (bitset bset1, bitset bset2)
 }
 
 
-/* Find previous bit set in SRC starting from and including BITNO.
-   Return BITSET_BINDEX_MAX if SRC empty.  */
 bitset_bindex
 bitset_prev (bitset src, bitset_bindex bitno)
 {
diff --git a/lib/bitset.h b/lib/bitset.h
index a13b8d4d1..172b2c575 100644
--- a/lib/bitset.h
+++ b/lib/bitset.h
@@ -282,17 +282,21 @@ bitset_test (bitset bset, bitset_bindex bitno)
 /* Return true if both bitsets are of the same type and size.  */
 bool bitset_compatible_p (bitset bset1, bitset bset2);
 
-/* Find next set bit from the given bit index.  */
-bitset_bindex bitset_next (bitset, bitset_bindex);
+/* Find next bit set in SRC starting from and including BITNO.
+   Return BITSET_BINDEX_MAX if SRC empty.  */
+bitset_bindex bitset_next (bitset src, bitset_bindex bitno);
 
-/* Find previous set bit from the given bit index.  */
-bitset_bindex bitset_prev (bitset, bitset_bindex);
+/* Find previous bit set in SRC starting from and including BITNO.
+   Return BITSET_BINDEX_MAX if SRC empty.  */
+bitset_bindex bitset_prev (bitset src, bitset_bindex bitno);
 
-/* Find first set bit.  */
-bitset_bindex bitset_first (bitset);
+/* Find first set bit.
+   Return BITSET_BINDEX_MAX if SRC empty.  */
+bitset_bindex bitset_first (bitset src);
 
-/* Find last set bit.  */
-bitset_bindex bitset_last (bitset);
+/* Find last set bit.
+   Return BITSET_BINDEX_MAX if SRC empty.  */
+bitset_bindex bitset_last (bitset src);
 
 /* Return nonzero if this is the only set bit.  */
 bool bitset_only_set_p (bitset, bitset_bindex);
diff --git a/lib/bitset/array.c b/lib/bitset/array.c
index f350b53eb..1db583891 100644
--- a/lib/bitset/array.c
+++ b/lib/bitset/array.c
@@ -42,7 +42,7 @@ abitset_resize (bitset src, bitset_bindex size)
   return size;
 }
 
-/* Find list of up to NUM bits set in BSET starting from and including
+/* Find list of up to NUM bits set in SRC starting from and including
*NEXT and store in array LIST.  Return with actual number of bits
found and with *NEXT indicating where search stopped.  */
 static bitset_bindex
@@ -130,7 +130,7 @@ abitset_test (bitset src MAYBE_UNUSED,
 }
 
 
-/* Find list of up to NUM bits set in BSET in reverse order, starting
+/* Find list of up to NUM bits set in SRC in reverse order, starting
from and including NEXT and store in array LIST.  Return with
actual number of bits found and with *NEXT indicating where search
stopped.  */
@@ -182,7 +182,7 @@ abitset_list_reverse (bitset src, bitset_bindex *list,
 }
 
 
-/* Find list of up to NUM bits set in BSET starting from and including
+/* Find list of up to NUM bits set in SRC starting from and including
*NEXT and store in array LIST.  Return with actual number of bits
found and with *NEXT indicating where search stopped.  */
 static bitset_bindex
-- 
2.29.2

[PATCH 4/5] bitset: more tests

[Akim Demaille]

These new tests managed to uncover shortcomings in previous versions
of the following commit.

* tests/test-bitset.c (compare): Make it clear that the random values
should not be modified.
Check bitset_first, bitset_last and BITSET_FOR_EACH.
---
 ChangeLog   |   5 +++
 tests/test-bitset.c | 107 
 2 files changed, 103 insertions(+), 9 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index ff21b6631..713206bcf 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,10 @@
 2020-11-15  Akim Demaille  
 
+   bitset: more tests
+   * tests/test-bitset.c (compare): Make it clear that the random values
+   should not be modified.
+   Check bitset_first, bitset_last and BITSET_FOR_EACH.
+
bitset: fix the copy from lbitset to other types
* lib/bitset/list.c (lbitset_copy): Rename as...
(lbitset_copy_): this.
diff --git a/tests/test-bitset.c b/tests/test-bitset.c
index 02d7cda82..523210dae 100644
--- a/tests/test-bitset.c
+++ b/tests/test-bitset.c
@@ -48,24 +48,28 @@ void compare (enum bitset_attr a, enum bitset_attr b)
 {
   const int nbits = RANDOM (256);
 
-  bitset asrc0 = bitset_create (nbits, a);
+  /* Four read only random initial values of type A.  */
+  const bitset asrc0 = bitset_create (nbits, a);
   bitset_random (asrc0);
-  bitset asrc1 = bitset_create (nbits, a);
+  const bitset asrc1 = bitset_create (nbits, a);
   bitset_random (asrc1);
-  bitset asrc2 = bitset_create (nbits, a);
+  const bitset asrc2 = bitset_create (nbits, a);
   bitset_random (asrc2);
-  bitset asrc3 = bitset_create (nbits, a);
+  const bitset asrc3 = bitset_create (nbits, a);
   bitset_random (asrc3);
-  bitset adst = bitset_create (nbits, a);
 
-  bitset bsrc0 = bitset_create (nbits, b);
+  /* Four read only values of type B, equal to the values of type A. */
+  const bitset bsrc0 = bitset_create (nbits, b);
   bitset_copy (bsrc0, asrc0);
-  bitset bsrc1 = bitset_create (nbits, b);
+  const bitset bsrc1 = bitset_create (nbits, b);
   bitset_copy (bsrc1, asrc1);
-  bitset bsrc2 = bitset_create (nbits, b);
+  const bitset bsrc2 = bitset_create (nbits, b);
   bitset_copy (bsrc2, asrc2);
-  bitset bsrc3 = bitset_create (nbits, b);
+  const bitset bsrc3 = bitset_create (nbits, b);
   bitset_copy (bsrc3, asrc3);
+
+  /* Destinations for each operation.  */
+  bitset adst = bitset_create (nbits, a);
   bitset bdst = bitset_create (nbits, b);
 
   /* not */
@@ -139,6 +143,91 @@ void compare (enum bitset_attr a, enum bitset_attr b)
   bitset_zero (bdst);
   assert_bitset_equal (adst, bdst);
 
+  /* first and last
+
+ Exercise on random values (i == -1), but also on all the
+ single-bit values: it's easy to get the handling of the most
+ significant bit wrong.  */
+  for (int i = -1; i < nbits; ++i)
+{
+  /* Work on bdst to exercise all the bitset types (adst is
+ BITSET_VARIABLE).  */
+  if (i >= 0)
+{
+  bitset_zero (bdst);
+  bitset_set (bdst, i);
+}
+  else
+{
+  bitset_copy (bdst, bsrc0);
+  debug_bitset (bdst);
+  debug_bitset (bsrc0);
+}
+  bitset_copy (adst, bdst);
+
+  {
+  bitset_bindex first = bitset_first (adst);
+  ASSERT (first == bitset_first (bdst));
+
+  bitset_bindex last  = bitset_last (adst);
+  ASSERT (last == bitset_last (bdst));
+
+  if (i >= 0)
+{
+  ASSERT (first == i);
+  ASSERT (last == i);
+}
+
+  if (first != BITSET_BINDEX_MAX)
+{
+  ASSERT (last != BITSET_BINDEX_MAX);
+  ASSERT (first <= last);
+  ASSERT (bitset_test (adst, first));
+  ASSERT (bitset_test (adst, last));
+  ASSERT (bitset_test (bdst, first));
+  ASSERT (bitset_test (bdst, last));
+}
+  else
+ASSERT (last == BITSET_BINDEX_MAX);
+  }
+
+
+  /* FOR_EACH.
+
+ Iterate over bsrc0 to exercise all the bitset types (asrc0 is
+ BITSET_VARIABLE).  */
+  {
+bitset_iterator iter;
+bitset_bindex j;
+bitset_bindex first = bitset_first (bdst);
+bitset_bindex last  = bitset_last (bdst);
+bool seen_first = false;
+bool seen_last = false;
+BITSET_FOR_EACH (iter, bdst, j, 0)
+  {
+ASSERT (first <= j && j <= last);
+ASSERT (bitset_test (bdst, j));
+if (j == first)
+  seen_first = true;
+if (j == last)
+  seen_last = true;
+if (0 <= i)
+  ASSERT (j == i);
+  }
+if (first == BITSET_BINDEX_MAX)
+  {
+ASSERT (!seen_first);
+ASSERT (!seen_last);
+  }
+else
+  {
+ASSERT (seen_first);
+ASSERT (seen_last);
+  }
+  }
+  }
+
+
   /* resize.
 
  ARRAY bitsets cannot be resized.  */
-- 
2.29.2

[PATCH 0/5] bitset: use ffs

[Akim Demaille]

Some time ago, Bruno reported that it was surprising that there are no
occurrences of ffs in the bitset module.  This series introduces the
use of ffs in array-based bitsets.  There are more places where ffs
could be used, but before installing it everywhere, I want to first
make sure that the approach I took is ok.

Originally bitset right-shifting words looking for set
least-significant bits:

for (bitno = bitoff; word; bitno++)
  {
if (word & 1)
  list[count++] = bitno;
word >>= 1;
  }
  
I first tried to reproduce something similar with ffs.  Unfortunately
that led to `word >>= pos + 1`, where pos could be 63, so noop...

Instead, I'm reseting each least-significant bit in word, and the loop
above becomes:

BITSET_FOR_EACH_BIT (pos, word)
  list[count++] = bitoff + pos;

with

#define BITSET_FOR_EACH_BIT(Pos, Word)  \
  for (int Pos = bitset_ffs (Word); \
   0 <= Pos;\
   Word ^= 1UL << Pos, Pos = bitset_ffs (Word))

Cheers!

Akim Demaille (5):
  bitset: comment changes
  bitset: making debug traces more useful
  bitset: fix the copy from lbitset to other types
  bitset: more tests
  bitset: use ffsl to accelerate iterations over set bits

 ChangeLog   |  28 
 lib/bitset.c|   9 ++--
 lib/bitset.h|  28 
 lib/bitset/array.c  |  56 +--
 lib/bitset/base.h   |   9 
 lib/bitset/list.c   |  11 -
 modules/bitset  |   1 +
 tests/test-bitset.c | 107 
 8 files changed, 190 insertions(+), 59 deletions(-)

-- 
2.29.2

Re: Typo in quote.h

[Akim Demaille]

Hi Bruno, Paul and Reuben,

> Le 31 oct. 2020 à 20:41, Bruno Haible  a écrit :\
> 
> You are right that there is a problem here: On 2012-03-07 this patch [1]
> from Akim did a good thing — it made the quote.h functions' effect more
> customizable — and a not so good thing: it combined the implementations
> of two modules.

Yes, the point was to keep the struct truly private, unreachable.

I see gnulib already has both -private.h and -internal.h files.  I don't know 
if this is a case where we'd want to create such a file.

> Here are three proposed patches. Paul and Akim, is this OK?
> 
> 
> 2020-10-31  Bruno Haible  
> 
>   quote: Don't mix with the implementation of module 'quotearg'.
>   * lib/quote.c: New file, extracted from lib/quotearg.c.
>   * lib/quotearg.c: Don't include quote.h.
>   (quote_quoting_options, quote_n_mem, quote_mem, quote_n, quote): Remove.
>   * modules/quote (Files): Add lib/quote.c.
>   (Depends-on): Add stdint.
>   (Makefile.am): Compile quote.c.
>   * modules/quotearg (Files): Remove lib/quote.h.
> 
> 2020-10-31  Bruno Haible  
> 
>   quotearg: Export quotearg_slot_n_mem.
>   * lib/quotearg.h (quotearg_buffer, quotearg_alloc_mem): Write SIZE_MAX
>   instead of -1.
>   (quotearg_slot_n_mem): New declaration.
>   * lib/quotearg.c (quotearg_slot_n_mem): Renamed from quotearg_n_options.
>   Use default_quoting_options if the options argument is NULL.
> 
> 2020-10-31  Bruno Haible  
> 
>   quotearg: Allow static init of 'struct quoting_options' variables.
>   * lib/quotearg.h: Include .
>   (struct quoting_options): Move to here from lib/quotearg.c. Use
>   INT_WIDTH instead of INT_BITS.
>   (QUOTING_OPTIONS_INIT): New macro.
>   * lib/quotearg.c (INT_BITS): Remove macro. Use INT_WIDTH instead.
>   (default_quoting_options, quote_quoting_options): Use
>   QUOTING_OPTIONS_INIT.
>   * modules/quotearg (Depends-on): Add limits-h.

I'm fine with these changes, thanks!

> The fields of this struct are considered private.  Instead of accessing
> them directly, use the accessors

I would be more imperative

> The fields of this struct are private.  Don't access them directly, use the 
> accessors Luke

Cheers!

Re: Portability of libtextstyle

[Akim Demaille]

Hi Bruno!

> Le 1 août 2020 à 15:10, Bruno Haible  a écrit :
> 
> In Bison's configure.ac, you now need to write
>  gl_LIBTEXTSTYLE_OPTIONAL([0.20.5])
> This will guarantee that you can use ostream_printf. If an older version of
> libtextstyle is installed, it will not be used.
> 
> 
> 2020-08-01  Bruno Haible  
> 
>   libtextstyle[-optional]: Allow requesting a minimum version.
>   * m4/libtextstyle.m4 (gl_LIBTEXTSTYLE): Allow an optional argument.
>   (gl_LIBTEXTSTYLE_NEWEST_VERSION, gl_LIBTEXTSTYLE_INITIALIZE,
>   gl_LIBTEXTSTYLE_SEARCH): New macros.
>   * modules/libtextstyle (configure.ac): Don't invoke gl_LIBTEXTSTYLE.
>   * m4/libtextstyle-optional.m4 (gl_LIBTEXTSTYLE_OPTIONAL): Allow an
>   optional argument. Invoke, not require, gl_LIBTEXTSTYLE.
>   * modules/libtextstyle-optional (configure.ac): Don't invoke
>   gl_LIBTEXTSTYLE_OPTIONAL.
>   * NEWS: Mention the changes.

Excellent!  Thanks a lot for this!

Re: Portability of libtextstyle

[Akim Demaille]

Hi Bruno,

Thanks for coming back about this!

> Le 29 juil. 2020 à 11:08, Bruno Haible  a écrit :
> 
> Hi Akim,
> 
>> Here's my proposal to update the check for libtextstyle.
>> 
>> Cheers!
>> 
>> commit 76c00e218869dab03ec4a1f503495e0d729d0b97
>> Author: Akim Demaille 
>> Date:   Sun Jul 26 18:00:10 2020 +0200
>> 
>>libtextstyle: check the API of the latest release
>> 
>>* m4/libtextstyle.m4 (gl_LIBTEXTSTYLE): Require ostream_printf and
>>styled_ostream_get_hyperlink_ref.
> 
> Having thought a while about it... I would prefer that users have
> the possibility to request any libtextstyle vs. request a libtextstyle
> which defines ostream_printf.
> 
> There are two possibilities to implement that:
>  (a) Turn 'libtextstyle' into a module which, additionally, requires an 
> explicit
>  version number e.g. gl_LIBTEXTSTYLE([0.20.5]).
>  (b) Leave module 'libtextstyle' as it is, and define a new module
>  'libtextstyle-0.20.5' that does what you need. Likewise for
>  'libtextstyle-optional'.
> 
> Since (a) is not backward compatible

(a) can easily turned into something compatible by using the version number
of the current API as the default requirement.  Besides, in the context of
gnulib, backward compatibility is less of a problem, since there's an
explicit act from the user to move forward.

(b) does not seem to scale very well, unless you really believe that there
will be no need for more version numbers.

So I don't understand why you prefer (b), but I can perfectly live with this
solution.

> and also not the way the modules libsigsegv,
> libunistring, libgmp work, I would prefer (b).
> 
> But it would require that you change bison's bootstrap.conf. Would that be OK
> with you?

Sure!  That is fine with me.

How do we proceed?  Who does what?

Cheers!

Re: Portability of libtextstyle

[Akim Demaille]

Hi Bruno,

> Le 27 juil. 2020 à 03:17, Bruno Haible  a écrit :
> 
> Hi Akim,
> 
> I wrote:
>> I propose that I make a gettext release ASAP (today?), that includes the new
>> libtextstyle API, then you can refer all users to gettext-0.21.
> 
> I released gettext-0.21 now. You can tell your users that it is a
> prerequisite for proper styling support of bison-3.7.

Wow...  Thanks a lot for the effort!  I appreciate that.

Here's my proposal to update the check for libtextstyle.

Cheers!

commit 76c00e218869dab03ec4a1f503495e0d729d0b97
Author: Akim Demaille 
Date:   Sun Jul 26 18:00:10 2020 +0200

libtextstyle: check the API of the latest release

* m4/libtextstyle.m4 (gl_LIBTEXTSTYLE): Require ostream_printf and
styled_ostream_get_hyperlink_ref.

diff --git a/ChangeLog b/ChangeLog
index 63138b822..9e7512bf2 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2020-07-27  Akim Demaille  
+
+   libtextstyle: check the API of the latest release
+   * m4/libtextstyle.m4 (gl_LIBTEXTSTYLE): Require ostream_printf and
+   styled_ostream_get_hyperlink_ref.
+
 2020-07-26  Paul Eggert  
 
argz: pacify MSVC
diff --git a/m4/libtextstyle.m4 b/m4/libtextstyle.m4
index 23b4a6959..96ce60688 100644
--- a/m4/libtextstyle.m4
+++ b/m4/libtextstyle.m4
@@ -1,4 +1,4 @@
-# libtextstyle.m4 serial 1
+# libtextstyle.m4 serial 2
 dnl Copyright (C) 2019-2020 Free Software Foundation, Inc.
 dnl This file is free software; the Free Software Foundation
 dnl gives unlimited permission to copy and/or distribute it,
@@ -17,6 +17,11 @@ dnl LTLIBTEXTSTYLE to empty.
 AC_DEFUN([gl_LIBTEXTSTYLE],
 [
   AC_LIB_HAVE_LINKFLAGS([textstyle], [],
-[#include ], 
[term_styled_ostream_create(1,"",TTYCTL_AUTO,"");],
+[#include ],
+[const char *url = "https://www.gnu.org/software/gnulib/;;
+ styled_ostream_t ostream = term_styled_ostream_create (1, "", 
TTYCTL_AUTO, "");
+ styled_ostream_set_hyperlink (ostream, url, NULL);
+ ostream_printf (ostream, "%s%s", "gnu", "lib");
+ styled_ostream_set_hyperlink (ostream, NULL, NULL);],
 [no])
 ])

Re: Portability of libtextstyle

[Akim Demaille]

Hi Bruno,

Thanks for the quick answer!

> Le 26 juil. 2020 à 17:25, Bruno Haible  a écrit :
> 
> Hi Akim,
> 
> The new functions (for hyperlinks and ostream_printf) are contained in
> prerelease tarballs that I've been circulating, but not contained in any
> release so far.

I had not realized.

>> Another point is that I expected --without-libtextstyle-prefix to disable
>> explicitly libtextstyle, but apparently it doesn't
>> (https://lists.gnu.org/r/bug-bison/2020-07/msg00037.html).
> 
> For me, --without-libtextstyle-prefix works as expected:

Ah, the user must have done something wrong then, sorry about that.

>> I can submit changes once I know what options you prefer.
> 
> I propose that I make a gettext release ASAP (today?), that includes the new
> libtextstyle API, then you can refer all users to gettext-0.21.

I don't want to push you into a hurried release.  Anyway I think it's
more important for Bison to be able to avoid unsuitable versions of
libtextstyle than to have more widespread proper support.

I have to leave now, but I can send a patch first thing tomorrow.

Cheers!

Portability of libtextstyle

[Akim Demaille]

Hi Bruno,

I have been bitten by portability issues with libtextstyle: some people have 
versions that don't feature ostream_printf.  Unfortunately gnulib's Autoconf 
macros don't check for this function, so old versions of libtextstyle are 
accepted, and linking fails 
(https://lists.gnu.org/r/bug-bison/2020-07/msg00030.html).  I have no idea 
where the prototype was found by the way.

I don't know how you want to handle this situation:
1. gnulib macros should be able to be told what requirements one has one 
libtextstyle
2. gnulib macros always aim at the most recent "release" but checking for the 
most recent additions in the API.

I'd go for 2, but maybe you have a different opinion.

Another point is that I expected --without-libtextstyle-prefix to disable 
explicitly libtextstyle, but apparently it doesn't 
(https://lists.gnu.org/r/bug-bison/2020-07/msg00037.html).

I can submit changes once I know what options you prefer.

Cheers!

Re: portability issues with unicodeio

[Akim Demaille]

Hi Bruno,

> Le 9 juil. 2020 à 17:31, Bruno Haible  a écrit :
> 
> Kiyoshi KANAZAWA wrote:
>> make check passed both on Solaris 11.3 & 11.4.
> 
> Oh, I see. So my unit test was not complete.
> 
>>> It is succeeds this test, then what is the difference between the
>>> coreutils printf program and the test-unicodeio.c program? Both call
>>> setlocale (LC_ALL, "").
>> 
>> I do not understand.
>> What should I do next ?
> 
> I single-stepped both the coreutils printf program and the test-unicodeio.c
> program, and found the issue.
> 
> 
> 2020-07-09  Bruno Haible  
> 
>   unicodeio: Fix wrong result on Solaris 11.
>   Reported by Kiyoshi Kanazawa 
>   via Akim Demaille  in
>   <https://lists.gnu.org/archive/html/bug-gnulib/2020-07/msg00036.html>.
>   * lib/unicodeio.c (unicode_to_mb): Handle question mark fallback
>   characters on Solaris.
>   * tests/test-unicodeio.c (main): In the "C" locale, expect either the
>   UTF-8 output or the specified fallback.

Excellent!  Bruno, thanks a lot for this.

Yesterday evening I launched the process of releasing a new beta so
that Kiyoshi could try it, but did not send the announcement.  Yet
Kiyoshi managed to find out there was a new beta, and already confirmed
the issue is fixed.

Thanks to both of you.

Cheers!

portability issues with unicodeio (was: [GNU Bison 3.6.90] testsuite: 173 174 175 176 177 178 179 180 181 182 183 184 185 186 187 188 189 190 191 192 193 196 220 221 228 244 245 246 249 250 251 252 25

[Akim Demaille]

Hi!

Bison uses gnulib's unicodeio module to emit bullets (•) portably,
with a fallback to '.'.  It's implemented this way (src/gram.h):

> /* Fallback in case we can't print "•".  */
> static inline long
> print_dot_fallback (unsigned int code _GL_UNUSED,
> const char *msg _GL_UNUSED,
> void *callback_arg)
> {
>   FILE *out = (FILE *) callback_arg;
>   putc ('.', out);
>   return -1;
> }
> 
> /* Print "•", the symbol used to represent a point in an item (aka, a
>dotted rule).  */
> static inline void
> print_dot (FILE *out)
> {
>   unicode_to_mb (0x2022, fwrite_success_callback, print_dot_fallback, out);
> }

Unfortunately on Kiyoshi's environment (SunOS hidden 5.11 11.3 i86pc i386 i86pc,
GCC 9.3.0) we get '?' instead of '.' in the C locale.  We get a genuine ASCII
'?', it's not some fallback from the terminal which fails to display the
character.  And we properly get the bullet with en_US.UTF-8.

Kiyoshi can reproduce the problem with GNU Coreutils' printf, where he
get's a '?', although the fallback display the escape sequence (i.e.,
it should repeat '\u2022'):

> /* Simple failure callback that displays a fallback representation in plain
>ASCII, using the same notation as ISO C99 strings.  */
> static long
> fallback_failure_callback (unsigned int code,
>const char *msg _GL_UNUSED,
>void *callback_arg)
> {
>   FILE *stream = (FILE *) callback_arg;
> 
>   if (code < 0x1)
> fprintf (stream, "\\u%04X", code);
>   else
> fprintf (stream, "\\U%08X", code);
>   return -1;
> }
> 
> /* Outputs the Unicode character CODE to the output stream STREAM.
>Upon failure, exit if exit_on_error is true, otherwise output a fallback
>notation.  */
> void
> print_unicode_char (FILE *stream, unsigned int code, int exit_on_error)
> {
>   unicode_to_mb (code, fwrite_success_callback,
>  exit_on_error
>  ? exit_failure_callback
>  : fallback_failure_callback,
>  stream);
> }



Kiyoshi's messages start here:

https://lists.gnu.org/r/bug-bison/2020-07/msg1.html

The latest:

> Le 6 juil. 2020 à 22:35, Kiyoshi KANAZAWA  a 
> écrit :
> 
> Hi Akim,
> 
> $ LC_ALL=C $coreutilsbin/printf '\u2022\n' | od -t x1
> 000 3f 0a
> 002
> 
> $ LC_ALL=en_US.UTF-8 $coreutilsbin/printf '\u2022\n' | od -t x1
> 000 e2 80 a2 0a
> 004
> 
> 
> FYI, I have very limited locale.
> $ locale -a
> C
> POSIX
> en_US.ISO8859-1
> en_US.ISO8859-15
> en_US.ISO8859-15@euro
> en_US.UTF-8
> ja_JP.PCK
> ja_JP.UTF-8
> ja_JP.UTF-8@cldr
> ja_JP.eucJP

I'm unsure what the next steps would be from here.

Thanks in advance!

Re: list: fix GCC warnings

[Akim Demaille]

> Le 31 mai 2020 à 11:28, Bruno Haible  a écrit :
> 
> Hi Akim,
> 
> Thanks for caring about these warnings.

Sure!

>>* lib/gl_anytreehash_list2.h (gl_tree_search_from_to): Avoid using
>>the same variable name in nested scopes.
> 
> 'p' is a poor variable name here. Please, can use you 'nodes_elt' instead?

Good with me.

>> I don't think the MAYBE_UNUSED attribute may harm in any way, but if we
>> want to avoid that, the simplest would probably be to move the #if outside
>> and implement the function twice.
> 
> Better use _GL_ATTRIBUTE_MAYBE_UNUSED. Implementing the function twice means
>  - duplicating the parameter list,
>  - having no proper place for attaching a comment to the function.

Very much agreed.  Installed as follows.

Cheers!

commit d8a8fb3423499851bf06aac2302112944a276f97
Author: Akim Demaille 
Date:   Sun May 31 08:59:25 2020 +0200

list: fix GCC warnings

* lib/gl_anytree_list2.h (gl_tree_iterator_free)
(gl_tree_next_node, gl_tree_node_nx_set_value)
(gl_tree_previous_node, gl_tree_next_node):
Mark unused arguments.
* lib/gl_anytree_oset.h (gl_tree_iterator_free): Likewise.
* lib/gl_anylinked_list2.h (gl_linked_node_value)
(gl_linked_node_nx_set_value, gl_linked_iterator_free): Likewise.

* lib/gl_anytreehash_list2.h (gl_tree_search_from_to): Avoid using
the same variable name in nested scopes.

diff --git a/ChangeLog b/ChangeLog
index 321d5e28c..5ef05e037 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,17 @@
+2020-05-31  Akim Demaille  
+
+   list: fix GCC warnings
+   * lib/gl_anytree_list2.h (gl_tree_iterator_free)
+   (gl_tree_next_node, gl_tree_node_nx_set_value)
+   (gl_tree_previous_node, gl_tree_next_node):
+   Mark unused arguments.
+   * lib/gl_anytree_oset.h (gl_tree_iterator_free): Likewise.
+   * lib/gl_anylinked_list2.h (gl_linked_node_value)
+   (gl_linked_node_nx_set_value, gl_linked_iterator_free): Likewise.
+
+   * lib/gl_anytreehash_list2.h (gl_tree_search_from_to): Avoid using
+   the same variable name in nested scopes.
+
 2020-05-31  Bruno Haible  
 
list-c++, set-c++, oset-c++, map-c++, omap-c++: Don't fool the compiler.
diff --git a/lib/gl_anylinked_list2.h b/lib/gl_anylinked_list2.h
index 114106c7d..3e01f8fa0 100644
--- a/lib/gl_anylinked_list2.h
+++ b/lib/gl_anylinked_list2.h
@@ -76,11 +76,11 @@ gl_linked_nx_create_empty (gl_list_implementation_t 
implementation,
 
 static gl_list_t
 gl_linked_nx_create (gl_list_implementation_t implementation,
-  gl_listelement_equals_fn equals_fn,
-  gl_listelement_hashcode_fn hashcode_fn,
-  gl_listelement_dispose_fn dispose_fn,
-  bool allow_duplicates,
-  size_t count, const void **contents)
+ gl_listelement_equals_fn equals_fn,
+ gl_listelement_hashcode_fn hashcode_fn,
+ gl_listelement_dispose_fn dispose_fn,
+ bool allow_duplicates,
+ size_t count, const void **contents)
 {
   struct gl_list_impl *list =
 (struct gl_list_impl *) malloc (sizeof (struct gl_list_impl));
@@ -170,13 +170,15 @@ gl_linked_size (gl_list_t list)
 }
 
 static const void * _GL_ATTRIBUTE_PURE
-gl_linked_node_value (gl_list_t list, gl_list_node_t node)
+gl_linked_node_value (gl_list_t list _GL_ATTRIBUTE_MAYBE_UNUSED,
+  gl_list_node_t node)
 {
   return node->value;
 }
 
 static int
-gl_linked_node_nx_set_value (gl_list_t list, gl_list_node_t node,
+gl_linked_node_nx_set_value (gl_list_t list _GL_ATTRIBUTE_MAYBE_UNUSED,
+ gl_list_node_t node,
  const void *elt)
 {
 #if WITH_HASHTABLE
@@ -1021,7 +1023,7 @@ gl_linked_iterator_next (gl_list_iterator_t *iterator,
 }
 
 static void
-gl_linked_iterator_free (gl_list_iterator_t *iterator)
+gl_linked_iterator_free (gl_list_iterator_t *iterator 
_GL_ATTRIBUTE_MAYBE_UNUSED)
 {
 }
 
diff --git a/lib/gl_anytree_list2.h b/lib/gl_anytree_list2.h
index c5a67dbd0..939b79748 100644
--- a/lib/gl_anytree_list2.h
+++ b/lib/gl_anytree_list2.h
@@ -60,13 +60,15 @@ gl_tree_size (gl_list_t list)
 }
 
 static const void *
-gl_tree_node_value (gl_list_t list, gl_list_node_t node)
+gl_tree_node_value (gl_list_t list _GL_ATTRIBUTE_MAYBE_UNUSED,
+gl_list_node_t node)
 {
   return node->value;
 }
 
 static int
-gl_tree_node_nx_set_value (gl_list_t list, gl_list_node_t node, const void 
*elt)
+gl_tree_node_nx_set_value (gl_list_t list _GL_ATTRIBUTE_MAYBE_UNUSED,
+   gl_list_node_t node, const void *elt)
 {
 #if WITH_HASHTABLE
   if (elt != node->value)
@@ -101,7 +103,8 @@ gl_tree_node_nx_set_value (gl_list_t list, gl_list_node_t 
node, const void *elt)
 }
 
 static gl_list_node_t _GL_ATTRIBUTE_PURE
-gl_tree_next_node (gl_list_t list, gl_list_no

list: fix GCC warnings

[Akim Demaille]

Hi all,

There's one case debatable case: the functions that may actually
use the argument depending on macros.  For instance

static int
gl_linked_node_nx_set_value (gl_list_t list _GL_ATTRIBUTE_MAYBE_UNUSED,
 gl_list_node_t node,
 const void *elt)
{
#if WITH_HASHTABLE
  if (elt != node->value)
{
  size_t new_hashcode =
(list->base.hashcode_fn != NULL
 ? list->base.hashcode_fn (elt)
 : (size_t)(uintptr_t) elt);
...
}
#else
  node->value = elt;
#endif
  return 0;
}

I don't think the MAYBE_UNUSED attribute may harm in any way, but if we
want to avoid that, the simplest would probably be to move the #if outside
and implement the function twice.

Cheers!


commit a49be9d81e795a3f80bd08a4cfcdd74e9cd8b636
Author: Akim Demaille 
Date:   Sun May 31 08:59:25 2020 +0200

list: fix GCC warnings

* lib/gl_anytree_list2.h (gl_tree_iterator_free)
(gl_tree_next_node, gl_tree_node_nx_set_value)
(gl_tree_previous_node, gl_tree_next_node):
Mark unused arguments.
* lib/gl_anytree_oset.h (gl_tree_iterator_free): Likewise.
* lib/gl_anylinked_list2.h (gl_linked_node_value)
(gl_linked_node_nx_set_value, gl_linked_iterator_free): Likewise.

* lib/gl_anytreehash_list2.h (gl_tree_search_from_to): Avoid using
the same variable name in nested scopes.

diff --git a/ChangeLog b/ChangeLog
index e8133349c..7b2c01b68 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,17 @@
+2020-05-31  Akim Demaille  
+
+   list: fix GCC warnings
+   * lib/gl_anytree_list2.h (gl_tree_iterator_free)
+   (gl_tree_next_node, gl_tree_node_nx_set_value)
+   (gl_tree_previous_node, gl_tree_next_node):
+   Mark unused arguments.
+   * lib/gl_anytree_oset.h (gl_tree_iterator_free): Likewise.
+   * lib/gl_anylinked_list2.h (gl_linked_node_value)
+   (gl_linked_node_nx_set_value, gl_linked_iterator_free): Likewise.
+
+   * lib/gl_anytreehash_list2.h (gl_tree_search_from_to): Avoid using
+   the same variable name in nested scopes.
+
 2020-05-30  Bruno Haible  
 
wmemchr: Relicense under LGPLv2+.
diff --git a/lib/gl_anylinked_list2.h b/lib/gl_anylinked_list2.h
index 114106c7d..3e01f8fa0 100644
--- a/lib/gl_anylinked_list2.h
+++ b/lib/gl_anylinked_list2.h
@@ -76,11 +76,11 @@ gl_linked_nx_create_empty (gl_list_implementation_t 
implementation,
 
 static gl_list_t
 gl_linked_nx_create (gl_list_implementation_t implementation,
-  gl_listelement_equals_fn equals_fn,
-  gl_listelement_hashcode_fn hashcode_fn,
-  gl_listelement_dispose_fn dispose_fn,
-  bool allow_duplicates,
-  size_t count, const void **contents)
+ gl_listelement_equals_fn equals_fn,
+ gl_listelement_hashcode_fn hashcode_fn,
+ gl_listelement_dispose_fn dispose_fn,
+ bool allow_duplicates,
+ size_t count, const void **contents)
 {
   struct gl_list_impl *list =
 (struct gl_list_impl *) malloc (sizeof (struct gl_list_impl));
@@ -170,13 +170,15 @@ gl_linked_size (gl_list_t list)
 }
 
 static const void * _GL_ATTRIBUTE_PURE
-gl_linked_node_value (gl_list_t list, gl_list_node_t node)
+gl_linked_node_value (gl_list_t list _GL_ATTRIBUTE_MAYBE_UNUSED,
+  gl_list_node_t node)
 {
   return node->value;
 }
 
 static int
-gl_linked_node_nx_set_value (gl_list_t list, gl_list_node_t node,
+gl_linked_node_nx_set_value (gl_list_t list _GL_ATTRIBUTE_MAYBE_UNUSED,
+ gl_list_node_t node,
  const void *elt)
 {
 #if WITH_HASHTABLE
@@ -1021,7 +1023,7 @@ gl_linked_iterator_next (gl_list_iterator_t *iterator,
 }
 
 static void
-gl_linked_iterator_free (gl_list_iterator_t *iterator)
+gl_linked_iterator_free (gl_list_iterator_t *iterator 
_GL_ATTRIBUTE_MAYBE_UNUSED)
 {
 }
 
diff --git a/lib/gl_anytree_list2.h b/lib/gl_anytree_list2.h
index c5a67dbd0..939b79748 100644
--- a/lib/gl_anytree_list2.h
+++ b/lib/gl_anytree_list2.h
@@ -60,13 +60,15 @@ gl_tree_size (gl_list_t list)
 }
 
 static const void *
-gl_tree_node_value (gl_list_t list, gl_list_node_t node)
+gl_tree_node_value (gl_list_t list _GL_ATTRIBUTE_MAYBE_UNUSED,
+gl_list_node_t node)
 {
   return node->value;
 }
 
 static int
-gl_tree_node_nx_set_value (gl_list_t list, gl_list_node_t node, const void 
*elt)
+gl_tree_node_nx_set_value (gl_list_t list _GL_ATTRIBUTE_MAYBE_UNUSED,
+   gl_list_node_t node, const void *elt)
 {
 #if WITH_HASHTABLE
   if (elt != node->value)
@@ -101,7 +103,8 @@ gl_tree_node_nx_set_value (gl_list_t list, gl_list_node_t 
node, const void *elt)
 }
 
 static gl_list_node_t _GL_ATTRIBUTE_PURE
-gl_tree_next_node (gl_list_t list, gl_list_node_t node)
+gl_tree_next_node (gl_list_t list _GL_ATTRIBUTE_MAYBE_UNUSED,
+   gl_list_node_t

hash: add hash_xinsert

[Akim Demaille]

commit 6f3f39926b3de346695e6213b9378c643dc47817
Author: Akim Demaille 
Date:   Sun May 17 11:55:12 2020 +0200

hash: add hash_xinsert

* lib/hash.h, lib/xhash.c (hash_xinsert): New.

diff --git a/ChangeLog b/ChangeLog
index 889c756eb..65abbb559 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2020-05-17  Akim Demaille  
+
+   hash: add hash_xinsert
+   * lib/hash.h, lib/xhash.c (hash_xinsert): New.
+
 2020-05-16  Bruno Haible  
 
findprog-lgpl: Fix link error (existing since 2008-09-02).
diff --git a/lib/hash.h b/lib/hash.h
index ae08ce867..e5af43c0d 100644
--- a/lib/hash.h
+++ b/lib/hash.h
@@ -80,6 +80,7 @@ void hash_free (Hash_table *);
 /* Insertion and deletion.  */
 bool hash_rehash (Hash_table *, size_t) _GL_ATTRIBUTE_NODISCARD;
 void *hash_insert (Hash_table *, const void *) _GL_ATTRIBUTE_NODISCARD;
+void *hash_xinsert (Hash_table *, const void *);
 
 int hash_insert_if_absent (Hash_table *table, const void *entry,
const void **matched_ent);
diff --git a/lib/xhash.c b/lib/xhash.c
index 1e998d2d1..95df54501 100644
--- a/lib/xhash.c
+++ b/lib/xhash.c
@@ -36,3 +36,15 @@ hash_xinitialize (size_t candidate, const Hash_tuning 
*tuning,
 xalloc_die ();
   return res;
 }
+
+/* Same as hash_insert, but invokes xalloc_die on memory
+   exhaustion.  */
+
+void *
+hash_xinsert (Hash_table *table, void const *entry)
+{
+  void *res = hash_insert (table, entry);
+  if (!res)
+xalloc_die ();
+  return res;
+}

announce-gen: add support for dist-lzip

[Akim Demaille]

I installed this.  Cheers!

commit 4f4779e15adb7f1c61e8e13ab2bdf08974e33945
Author: Akim Demaille 
Date:   Sun May 10 18:11:04 2020 +0200

announce-gen: add support for dist-lzip

* build-aux/announce-gen (@archive_suffixes): Add tar.lz.

diff --git a/ChangeLog b/ChangeLog
index b31bbb185..1ba1c5f97 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2020-05-10  Akim Demaille  
+
+   announce-gen: add support for dist-lzip
+   * build-aux/announce-gen (@archive_suffixes): Add tar.lz.
+
 2020-05-09  Paul Eggert  
 
manywarnings: port to GCC 10.1
diff --git a/build-aux/announce-gen b/build-aux/announce-gen
index a59031f9e..b095a05eb 100755
--- a/build-aux/announce-gen
+++ b/build-aux/announce-gen
@@ -35,7 +35,7 @@
 eval 'exec perl -wSx "$0" "$@"'
  if 0;
 
-my $VERSION = '2020-04-04 15:07'; # UTC
+my $VERSION = '2020-05-10 16:13'; # UTC
 # The definition above must lie within the first 8 lines in order
 # for the Emacs time-stamp write hook (at end) to update it.
 # If you change this file with Emacs, please let the write hook
@@ -48,7 +48,7 @@ use POSIX qw(strftime);
 (my $ME = $0) =~ s|.*/||;
 
 my %valid_release_types = map {$_ => 1} qw (alpha beta stable);
-my @archive_suffixes = ('tar.gz', 'tar.bz2', 'tar.lzma', 'tar.xz');
+my @archive_suffixes = qw (tar.gz tar.bz2 tar.lz tar.lzma tar.xz);
 my %digest_classes =
   (
'md5' => (eval { require Digest::MD5; } and 'Digest::MD5'),

Re: m4: use AC_SUBST with two arguments when applicable

[Akim Demaille]

Hi Bruno,

> Le 10 mai 2020 à 14:17, Bruno Haible  a écrit :
> 
> Hi Akim,
> 
>> gl_SUBSTS([
>>  GNULIB_ISWBLANK=0
>>  GNULIB_ISWDIGIT=0
>>  GNULIB_ISWXDIGIT=0
>>  GNULIB_WCTYPE=0
>>  GNULIB_ISWCTYPE=0
>>  GNULIB_WCTRANS=0
>>  GNULIB_TOWCTRANS=0
>> ])
>> 
>> which does not, to my eyes, suffer from all the defects you see.
> 
> Yes, it does not degrade the ability to search for assignments.
> 
> But it goes against the general processing of Autoconf:
> 
> For an Autoconf macro, the input consists of identifier and shell statements.
> The output is shell, and the shell statements given as arguments appear
> in the output, unmodified except for m4 substitutions. Instead of a simple
> shell statement, I can always use a complex shell statement instead. E.g.
>  AM_CONDITIONAL([WINDOWS_NATIVE], [test $is_windows_native = yes])
> 
> gl_SUBSTS would analyze its shell statement arguments. Other Autoconf
> macros don't do this.

Some do.  AC_CHECK_MEMBER comes to my mind.

> It is not clear what would be the result of
>  gl_SUBSTS([
>if test $use_ctype_modules = yes; then
>  GNULIB_ISBLANK=0
>fi
>if test $use_wctype_modules = yes; then
>  GNULIB_ISWBLANK=0
>fi
>  ])

That would be ill-defined.  We could even make it a compile time error.

Re: m4: use AC_SUBST with two arguments when applicable

[Akim Demaille]

Hi Bruno!

> Le 9 mai 2020 à 22:14, Bruno Haible  a écrit :
> 
> Hi Akim,
> 
> (A) > I'm looking for means to avoid repetitions, and boilerplate.
> 
> It needs to be balanced against the other characteristics of maintainability.
> Two important aspects are:
>  (B) Making the code easy to understand.
>  (C) Making it easy to find all definitions and all references of an 
> identifier.

[...]

> Until the day I had to debug a complex invocation of a complex, hand-crafted,
> scarcely documented macro. I even had an IDE that showed me the result of the
> macro-expansion when I modified the input; nevertheless, since then, I tend
> to value (B) and (C) as more important than (A).

I fully subscribe to what you are reporting here.  I have suffered from
equivalent symptoms in other languages, and also caused that suffering to
others myself.

Yet in the present case, I do not see it as clearcut as you do.  I
wholeheartedly agree with the 'git grep' argument.  That's why I had
last proposed

gl_SUBSTS([
  GNULIB_ISWBLANK=0
  GNULIB_ISWDIGIT=0
  GNULIB_ISWXDIGIT=0
  GNULIB_WCTYPE=0
  GNULIB_ISWCTYPE=0
  GNULIB_WCTRANS=0
  GNULIB_TOWCTRANS=0
])

which does not, to my eyes, suffer from all the defects you see.  I view
AC_SUBST as meta data, not as actual substitutions, and I don't see a problem
with factoring such meta data.  I do like the clarity and regularity it
provides, including the case of very long identifier which are currently
treated on two lines, whereas most of them are one a single one.

Thanks for having taken the time to explain your point of view in details!

Cheers!

Akim

Re: m4: use AC_SUBST with two arguments when applicable

[Akim Demaille]

Hi Bruno,

> Le 9 mai 2020 à 17:25, Bruno Haible  a écrit :
> 
> Hi Akim,
> 
>>  AC_SUBST([GNULIB_ISWBLANK], [0])
>>  AC_SUBST([GNULIB_ISWDIGIT], [0])
>>  AC_SUBST([GNULIB_ISWXDIGIT], [0])
>>  AC_SUBST([GNULIB_WCTYPE], [0])
>>  AC_SUBST([GNULIB_ISWCTYPE], [0])
>>  AC_SUBST([GNULIB_WCTRANS], [0])
>>  AC_SUBST([GNULIB_TOWCTRANS], [0])
> 
> No, no, noo!! This would make the *.m4 code even harder to maintain.

:)

>>  gl_SUBSTS(
>>[[GNULIB_ISWBLANK], [0]],
>>[[GNULIB_ISWDIGIT], [0]],
>>[[GNULIB_ISWXDIGIT], [0]],
>>[[GNULIB_WCTYPE], [0]],
>>[[GNULIB_ISWCTYPE], [0]],
>>[[GNULIB_WCTRANS], [0]],
>>[[GNULIB_TOWCTRANS], [0]]
>>  )
> 
> Eeek! This is even worse!!!

:)

> Really, you need to consider two things when proposing new coding patterns:
> 
> 1) The semantics.
> 2) The way a programmer works with the code on a daily basis.
> 
> 1) Let's take an example: HAVE_OPENDIR. See how it's referenced in the *.m4
> files:
> 
> $ grep -rw HAVE_OPENDIR m4 modules
> m4/opendir.m4:HAVE_OPENDIR=0
> m4/opendir.m4:  if test $HAVE_OPENDIR = 1; then
> m4/opendir.m4:  case $host_os,$HAVE_OPENDIR in
> m4/dirent_h.m4:  HAVE_OPENDIR=1;   AC_SUBST([HAVE_OPENDIR])
> modules/opendir:filename[test $HAVE_OPENDIR = 0 || test 
> $REPLACE_OPENDIR = 1]
> modules/opendir:unistd  [test $HAVE_OPENDIR = 0 || test 
> $REPLACE_OPENDIR = 1]
> modules/opendir:closedir[test $HAVE_OPENDIR = 0 || test 
> $REPLACE_OPENDIR = 1]
> modules/opendir:dirfd   [test $HAVE_OPENDIR = 0 || test 
> $REPLACE_OPENDIR = 1]
> modules/opendir:if test $HAVE_OPENDIR = 0 || test $REPLACE_OPENDIR = 1; then
> modules/dirent:   -e 's/@''HAVE_OPENDIR''@/$(HAVE_OPENDIR)/g' \
> 
> What you see is:
>  - 2 assignments, at different places (even in different files),
>  - 2 references as shell variables in *.m4 files,
>  - more references as shell variables in the modules description,
>  - 1 AC_SUBST, so it becomes usable as @HAVE_OPENDIR@ in *.h files.
> 
> What you propose is
>  - arbitrary, because it selects one out of several assignments, to
>combine them with AC_SUBST,
>  - counter-maintainable, because it hides the fact that one of these
>is a variable assignment. When I have a shell variable, I want to
>have *all* assignments done through the VAR=... syntax and *all*
>references through $VAR syntax.

If what your pattern of work is grep foo=, then we could have

 gl_SUBSTS(
   GNULIB_ISWBLANK=0
   GNULIB_ISWDIGIT=0
   GNULIB_ISWXDIGIT=0
   GNULIB_WCTYPE=0
   GNULIB_ISWCTYPE=0
   GNULIB_WCTRANS=0
   GNULIB_TOWCTRANS=0
 )

But the current solution where you repeat things is not something I like.  
You're the boss, granted, your taste matters more than mine, granted.  But I 
dislike these repetitive and tedious lines.

> 2) I work with this code on a daily basis. Often I need to locate all
>   assignments. A 'grep -rw HAVE_OPENDIR=' does the job. What you
>   propose, would force me to search for
> 1. the 'HAVE_OPENDIR=' pattern,
> 2. the 'AC_SUBST([HAVE_OPENDIR]' pattern,
>   thus effectively doubling the time I need to make such a search.
> 
> And finally, your proposal with gl_SUBSTS groups things together that
> are semantically unrelated. There is no relation between GNULIB_ISWBLANK
> and GNULIB_WCTYPE. Therefore it is wrong to group them together.

The grouping is already there, just with more boilerplate.


> Most probably, you wanted to reduce the complexity of the *.m4 code by
> making it smaller?
> 
> A laudible goal, but unfortunately you have a totally wrong complexity
> measure.

Let's say that I would not put it this way...  But I agree we disagree.

> The complexity comes from the fact that there are different
> assignment in different files. But you can't reduce that complexity
> (or at least, I see no way of doing that).
> 
> --- --- --- ---
> 
> IF you want to reduce complexity in relation to AC_SUBST, then add
> a variant AC_SUBST_CONST in such a way that
>  - AC_SUBST_CONST(VAR) registers a substitution of VAR, like AC_SUBST
>does,
>  - AC_SUBST_CONST(VAR) asserts that VAR has already its value at this
>point,
>  - assignments to VAR that occur later are forbidden (like with 'const'
>variables in C).
> 
> This would reduce complexity, because it guarantees that the value is
> already known, which - by the way AC_REQUIRE works - means that no other
> file can modify the value.
> 
> For the maintainer, in those cases where AC_SUBST_CONST is applicable
> (e.g. HAVE_GLOB_H), it would greatly reduce the scope in which I have to
> look for assignments to the variable.
> 
> That's how you reduce complexity: Take away irrelevant freedoms, and
> make things more "functional" or "declarative" instead of "procedural".

I'm looking for means to avoid repetitions, and boilerplate.  This does
not.

Checking that the assigned value is "const" is nice though.  Yet I would
still prefer AC_SUBST_CONST([FOO=1]) than all these error prone repetitions.

m4: use AC_SUBST with two arguments when applicable

[Akim Demaille]

Actually, given the number of times this pattern is used, I would personally 
introduce gl_SUBSTS, a variadic version of AC_SUBST, to avoid all this 
repetition, say something like

  gl_SUBSTS(
[[GNULIB_ISWBLANK], [0]],
[[GNULIB_ISWDIGIT], [0]],
[[GNULIB_ISWXDIGIT], [0]],
[[GNULIB_WCTYPE], [0]],
[[GNULIB_ISWCTYPE], [0]],
[[GNULIB_WCTRANS], [0]],
[[GNULIB_TOWCTRANS], [0]]
  )

instead of

  AC_SUBST([GNULIB_ISWBLANK], [0])
  AC_SUBST([GNULIB_ISWDIGIT], [0])
  AC_SUBST([GNULIB_ISWXDIGIT], [0])
  AC_SUBST([GNULIB_WCTYPE], [0])
  AC_SUBST([GNULIB_ISWCTYPE], [0])
  AC_SUBST([GNULIB_WCTRANS], [0])
  AC_SUBST([GNULIB_TOWCTRANS], [0])

I can take care of that if there is agreement.

Ok to install?

commit 295dc196c4f5e186a7a435fa4c7535943e815132
Author: Akim Demaille 
Date:   Sat May 9 14:08:12 2020 +0200

m4: use AC_SUBST with two arguments when applicable.

* m4/arpa_inet_h.m4, m4/dirent_h.m4, m4/fcntl_h.m4,
* m4/fnmatch_h.m4, m4/iconv_h.m4, m4/inttypes.m4, m4/langinfo_h.m4,
* m4/locale_h.m4, m4/math_h.m4, m4/monetary_h.m4, m4/netdb_h.m4,
* m4/poll_h.m4, m4/pthread_h.m4, m4/pty_h.m4, m4/sched_h.m4,
* m4/search_h.m4, m4/signal_h.m4, m4/spawn_h.m4, m4/stddef_h.m4,
* m4/stdio_h.m4, m4/stdlib_h.m4, m4/string_h.m4, m4/sys_file_h.m4,
* m4/sys_ioctl_h.m4, m4/sys_resource_h.m4, m4/sys_select_h.m4,
* m4/sys_socket_h.m4, m4/sys_stat_h.m4, m4/sys_time_h.m4,
* m4/sys_utsname_h.m4, m4/termios_h.m4, m4/threads.m4, m4/time_h.m4,
* m4/uchar.m4, m4/unistd_h.m4, m4/utime_h.m4, m4/wchar_h.m4,
* m4/wctype_h.m4:
Use AC_SUBST([FOO], [BAR]) instead of FOO=BAR; AC_SUBST([FOO]).

diff --git a/ChangeLog b/ChangeLog
index bc99924b1..4cc9d30cd 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,19 @@
+2020-05-09  Akim Demaille  
+
+   m4: use AC_SUBST with two arguments when applicable.
+   * m4/arpa_inet_h.m4, m4/dirent_h.m4, m4/fcntl_h.m4,
+   * m4/fnmatch_h.m4, m4/iconv_h.m4, m4/inttypes.m4, m4/langinfo_h.m4,
+   * m4/locale_h.m4, m4/math_h.m4, m4/monetary_h.m4, m4/netdb_h.m4,
+   * m4/poll_h.m4, m4/pthread_h.m4, m4/pty_h.m4, m4/sched_h.m4,
+   * m4/search_h.m4, m4/signal_h.m4, m4/spawn_h.m4, m4/stddef_h.m4,
+   * m4/stdio_h.m4, m4/stdlib_h.m4, m4/string_h.m4, m4/sys_file_h.m4,
+   * m4/sys_ioctl_h.m4, m4/sys_resource_h.m4, m4/sys_select_h.m4,
+   * m4/sys_socket_h.m4, m4/sys_stat_h.m4, m4/sys_time_h.m4,
+   * m4/sys_utsname_h.m4, m4/termios_h.m4, m4/threads.m4, m4/time_h.m4,
+   * m4/uchar.m4, m4/unistd_h.m4, m4/utime_h.m4, m4/wchar_h.m4,
+   * m4/wctype_h.m4:
+   Use AC_SUBST([FOO], [BAR]) instead of FOO=BAR; AC_SUBST([FOO]).
+
 2020-05-09  Akim Demaille  
 
bitset: use the attribute module
diff --git a/m4/arpa_inet_h.m4 b/m4/arpa_inet_h.m4
index b39449494..3f0562b05 100644
--- a/m4/arpa_inet_h.m4
+++ b/m4/arpa_inet_h.m4
@@ -1,4 +1,4 @@
-# arpa_inet_h.m4 serial 14
+# arpa_inet_h.m4 serial 15
 dnl Copyright (C) 2006, 2008-2020 Free Software Foundation, Inc.
 dnl This file is free software; the Free Software Foundation
 dnl gives unlimited permission to copy and/or distribute it,
@@ -49,11 +49,11 @@ AC_DEFUN([gl_ARPA_INET_MODULE_INDICATOR],
 
 AC_DEFUN([gl_ARPA_INET_H_DEFAULTS],
 [
-  GNULIB_INET_NTOP=0; AC_SUBST([GNULIB_INET_NTOP])
-  GNULIB_INET_PTON=0; AC_SUBST([GNULIB_INET_PTON])
+  AC_SUBST([GNULIB_INET_NTOP], [0])
+  AC_SUBST([GNULIB_INET_PTON], [0])
   dnl Assume proper GNU behavior unless another module says otherwise.
-  HAVE_DECL_INET_NTOP=1;  AC_SUBST([HAVE_DECL_INET_NTOP])
-  HAVE_DECL_INET_PTON=1;  AC_SUBST([HAVE_DECL_INET_PTON])
-  REPLACE_INET_NTOP=0;AC_SUBST([REPLACE_INET_NTOP])
-  REPLACE_INET_PTON=0;AC_SUBST([REPLACE_INET_PTON])
+  AC_SUBST([HAVE_DECL_INET_NTOP], [1])
+  AC_SUBST([HAVE_DECL_INET_PTON], [1])
+  AC_SUBST([REPLACE_INET_NTOP],   [0])
+  AC_SUBST([REPLACE_INET_PTON],   [0])
 ])
diff --git a/m4/dirent_h.m4 b/m4/dirent_h.m4
index 8bef6a0ce..93130618f 100644
--- a/m4/dirent_h.m4
+++ b/m4/dirent_h.m4
@@ -1,4 +1,4 @@
-# dirent_h.m4 serial 16
+# dirent_h.m4 serial 17
 dnl Copyright (C) 2008-2020 Free Software Foundation, Inc.
 dnl This file is free software; the Free Software Foundation
 dnl gives unlimited permission to copy and/or distribute it,
@@ -39,26 +39,26 @@ AC_DEFUN([gl_DIRENT_MODULE_INDICATOR],
 AC_DEFUN([gl_DIRENT_H_DEFAULTS],
 [
   AC_REQUIRE([gl_UNISTD_H_DEFAULTS]) dnl for REPLACE_FCHDIR
-  GNULIB_OPENDIR=0; AC_SUBST([GNULIB_OPENDIR])
-  GNULIB_READDIR=0; AC_SUBST([GNULIB_READDIR])
-  GNULIB_REWINDDIR=0;   AC_SUBST([GNULIB_REWINDDIR])
-  GNULIB_CLOSEDIR=0;AC_SUBST([GNULIB_CLOSEDIR])
-  GNULIB_DIRFD=0;   AC_SUBST([GNULIB_DIRFD])
-  GNULIB_FDOPENDIR=0;   AC_SUBST([GNULIB_FDOPENDIR])
-  GNULIB_SCANDIR=0; AC_SUBST([GNULIB_SCANDIR])
-  GNULIB_ALPHASORT=0;   AC_SUBST([GNULIB_ALPHASORT])
+  AC_SUBST([GNULIB_OPENDIR],   [0])
+  AC_SUBST([GNULIB_READDIR],   [0])
+  AC_SUBST([GNULIB_REWINDDIR], [0])
+  AC_SUBST([GNULIB_CLOSEDIR

Re: The attribute module

[Akim Demaille]

> Le 9 mai 2020 à 13:24, Bruno Haible  a écrit :
> 
> Hi Akim,
> 
>>> In file included from ../lib/bitsetv.c:21:
>>> In file included from ../lib/bitsetv.h:24:
>>> In file included from ../lib/bitset.h:31:
>>> In file included from ../lib/bitset/base.h:28:
>>> ../lib/xalloc.h:43:10: warning: '_GL_ATTRIBUTE_ALLOC_SIZE' macro redefined 
>>> [-Wmacro-redefined]
>>> # define _GL_ATTRIBUTE_ALLOC_SIZE(args)
>>> ^
>>> ./lib/config.h:1657:10: note: previous definition is here
>>> # define _GL_ATTRIBUTE_ALLOC_SIZE(args) __attribute__ ((__alloc_size__ 
>>> args))
>>> ^
> 
> These two patches should fix these warnings.

Ah, I had the second one done too :)

So there remains this one.  Ok to install?



0002-bitset-use-the-attribute-module.patch
Description: Binary data

Re: The attribute module

[Akim Demaille]

Hi Bruno,

> Le 9 mai 2020 à 12:37, Bruno Haible  a écrit :
> 
> Hi Akim,
> 
> Also, I avoid to use _GL_ATTRIBUTE_FORMAT and ATTRIBUTE_FORMAT, because of a
> tricky override of 'printf' by '__printf__', done in gnulib and libintl.

Then, shouldn't gnulib expose the _GL_ATTRIBUTE_FORMAT_(PRINTF|SCANF)(_SYSTEM)? 
macros as defined in stdio.in.h?

The attribute module

[Akim Demaille]

Hi all,

Bison started to use the new attribute module, but it generates conflicts with 
other modules, because one definition comes from config.h, and another from the 
other module's file.  For instance:

> In file included from ../lib/bitsetv.c:21:
> In file included from ../lib/bitsetv.h:24:
> In file included from ../lib/bitset.h:31:
> In file included from ../lib/bitset/base.h:28:
> ../lib/xalloc.h:43:10: warning: '_GL_ATTRIBUTE_ALLOC_SIZE' macro redefined 
> [-Wmacro-redefined]
> # define _GL_ATTRIBUTE_ALLOC_SIZE(args)
>  ^
> ./lib/config.h:1657:10: note: previous definition is here
> # define _GL_ATTRIBUTE_ALLOC_SIZE(args) __attribute__ ((__alloc_size__ args))
>  ^

I can provide a patch to address this, but I don't know what's the guideline 
here.  Should (virtually) all the modules eventually depend on "attribute" and 
stop defining these macros themselves, or should their macro definitions be 
robust to the now possibly existing macros?

Also, what's the rule inside gnulib to decide whether to use _GL_ATTRIBUTE_FOO 
rather than using attribute.h and ATTRIBUTE_FOO?

Cheers!

Re: bison: new module

[Akim Demaille]

> Le 3 mai 2020 à 12:02, Bruno Haible  a écrit :
> 
>> How about this?
> 
> Looks good.
> 
> After you pushed it, I'll test it from within GNU gettext.

Pushed.

Re: bison: new module

[Akim Demaille]

Hi Bruno,

> Le 3 mai 2020 à 11:35, Bruno Haible  a écrit :
> 
> Hi Akim,
> 
>> I didn't see the need for a temporary directory.
> 
> I didn't talk about a temporary directory, only about a temporary file.

Doh...  Sorry.

> The patch looks nearly good.
> 
>> +if _AC_DO_VAR([$1 conftest.y -o conftest.c]); then
> 
> I would try to avoid the use of undocumented Autoconf internals. Is that
> possible here?

Sure.  The use of _AC_DO* makes it visible in config.log, but that
is not a requirement.

How about this?

commit 3db0ae5f4fd5948baa6c133e01892aebc9db844f
Author: Akim Demaille 
Date:   Sun May 3 08:54:58 2020 +0200

bison: rely on bison's %require to check a version requirement

See https://lists.gnu.org/archive/html/bug-gnulib/2020-05/msg00021.html.

* m4/bison.m4 (gl_PROG_BISON): Let bison itself decide if it it recent
enough of not.
So far it is the only know Yacc tool that supports '%require'.
Other yaccs will actually even choke on seeing the -o option after the
input file name.
* m4/parse-datetime.m4: Simplify gl_PROG_BISON invocation.

diff --git a/ChangeLog b/ChangeLog
index 0bf6ca0dd..1ac3d104f 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,15 @@
+2020-05-03  Akim Demaille  
+
+   bison: rely on bison's %require to check a version requirement
+   See https://lists.gnu.org/archive/html/bug-gnulib/2020-05/msg00021.html.
+   * m4/bison.m4 (gl_PROG_BISON): Let bison itself decide if it it recent
+   enough of not.
+   So far it is the only know Yacc tool that supports '%require'.
+   Other yaccs will actually even choke on seeing the -o option after the
+   input file name.
+   * m4/parse-datetime.m4: Simplify gl_PROG_BISON invocation.
+
+
 2020-05-02  Bruno Haible  
 
list: Add get_first, get_last, set_first, set_last operations.
diff --git a/m4/bison.m4 b/m4/bison.m4
index 5802a5ca5..76b62baf8 100644
--- a/m4/bison.m4
+++ b/m4/bison.m4
@@ -1,4 +1,4 @@
-# serial 8
+# serial 9
 
 # Copyright (C) 2002-2006, 2008-2020 Free Software Foundation, Inc.
 # This file is free software; the Free Software Foundation
@@ -25,9 +25,9 @@
 # - %define api.pure   requires bison 2.7 or newer,
 # - %precedencerequires bison 3.0 or newer.
 #   For these, in the configure.ac you will need an invocation of
-# gl_PROG_BISON([VARIABLE], [MIN_BISON_VERSION], 
[BISON_VERSIONS_TO_EXCLUDE])
+# gl_PROG_BISON([VARIABLE], [MIN_BISON_VERSION])
 #   Example:
-# gl_PROG_BISON([PARSE_DATETIME_BISON], [2.4], [1.* | 2.[0-3] | 2.[0-3].*])
+# gl_PROG_BISON([PARSE_DATETIME_BISON], [2.4])
 #   With this preparation, in the Makefile.am there are two ways to formulate
 #   the invocation. Both are direct, without use of 'ylwrap'.
 #   (a) You can invoke
@@ -47,18 +47,23 @@ AC_DEFUN([gl_PROG_BISON],
   if test -z "$[$1]"; then
 ac_verc_fail=yes
   else
-dnl Found it, now check the version.
-AC_MSG_CHECKING([version of bison])
-changequote(<<,>>)dnl
-ac_prog_version=`$<<$1>> --version 2>&1 | sed -n 's/^.*GNU Bison.* 
\([0-9]*\.[0-9.]*\).*$/\1/p'`
-case $ac_prog_version in
-  '') ac_prog_version="v. ?.??, bad"; ac_verc_fail=yes;;
-  <<$3>>)
- ac_prog_version="$ac_prog_version, bad"; ac_verc_fail=yes;;
-  *) ac_prog_version="$ac_prog_version, ok"; ac_verc_fail=no;;
-esac
-changequote([,])dnl
+cat >conftest.y <<_ACEOF
+%require "$2"
+%%
+exp:
+_ACEOF
+AC_MSG_CHECKING([for bison $2 or newer])
+ac_prog_version=`$$1 --version 2>&1 | sed -n 's/^.*GNU Bison.* 
\([[0-9]]*\.[[0-9.]]*\).*$/\1/p'`
+: ${ac_prog_version:='v. ?.??'}
+if $$1 conftest.y -o conftest.c 2>/dev/null; then
+  ac_prog_version="$ac_prog_version, ok"
+  ac_verc_fail=false
+else
+  ac_prog_version="$ac_prog_version, bad"
+  ac_verc_fail=true
+fi
 AC_MSG_RESULT([$ac_prog_version])
+rm -f conftest.y conftest.c
   fi
   if test $ac_verc_fail = yes; then
 [$1]=:
diff --git a/m4/parse-datetime.m4 b/m4/parse-datetime.m4
index 0849b8203..3bb487f9b 100644
--- a/m4/parse-datetime.m4
+++ b/m4/parse-datetime.m4
@@ -35,7 +35,7 @@ AC_DEFUN([gl_PARSE_DATETIME],
   dnl the files or have a broken "make" program, hence the parse-datetime.c
   dnl rule will sometimes fire. To avoid an error, defines PARSE_DATETIME_BISON
   dnl to ":" if it is not present or too old.
-  gl_PROG_BISON([PARSE_DATETIME_BISON], [2.4], [1.* | 2.[0-3] | 2.[0-3].*])
+  gl_PROG_BISON([PARSE_DATETIME_BISON], [2.4])
 
   dnl Prerequisites of lib/parse-datetime.h.
   AC_REQUIRE([AM_STDBOOL_H])

Re: bison: new module

[Akim Demaille]

Hi Bruno,

> Le 2 mai 2020 à 19:50, Akim Demaille  a écrit :
> 
>> At configure time, both solutions are nearly on par: the time to
>> create a temporary file is negligible. And both will fail the same
>> way for a 'bison' program that references missing shared libraries.
>> 
>> Patch welcome!
> 
> Will do.

Here is my proposal.  I didn't see the need for a temporary directory.

Cheers!

commit 9011943076adc1885d57626763bb00276b7243af
Author: Akim Demaille 
Date:   Sun May 3 08:54:58 2020 +0200

bison: rely on bison's %require to check a version requirement

See https://lists.gnu.org/archive/html/bug-gnulib/2020-05/msg00021.html.

* m4/bison.m4 (gl_PROG_BISON): Let bison itself decide if it it recent
enough of not.
So far it is the only know Yacc tool that supports '%require'.
Other yaccs will actually even choke on seeing the -o option after the
input file name.
* m4/parse-datetime.m4: Simplify gl_PROG_BISON invocation.

commit 8d9b4d6e105571814d374d0d4689e75554f22902
Author: Akim Demaille 
Date:   Sun May 3 08:54:58 2020 +0200

bison: rely on bison's %require to check a version requirement

See https://lists.gnu.org/archive/html/bug-gnulib/2020-05/msg00021.html.

* m4/bison.m4 (gl_PROG_BISON): Let bison itself decide if it it recent
enough of not.
So far it is the only know Yacc tool that supports '%require'.
Other yaccs will actually even choke on seeing the -o option after the
input file name.
* m4/parse-datetime.m4: Simplify gl_PROG_BISON invocation.

diff --git a/ChangeLog b/ChangeLog
index 0bf6ca0dd..1ac3d104f 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,15 @@
+2020-05-03  Akim Demaille  
+
+   bison: rely on bison's %require to check a version requirement
+   See https://lists.gnu.org/archive/html/bug-gnulib/2020-05/msg00021.html.
+   * m4/bison.m4 (gl_PROG_BISON): Let bison itself decide if it it recent
+   enough of not.
+   So far it is the only know Yacc tool that supports '%require'.
+   Other yaccs will actually even choke on seeing the -o option after the
+   input file name.
+   * m4/parse-datetime.m4: Simplify gl_PROG_BISON invocation.
+
+
 2020-05-02  Bruno Haible  
 
list: Add get_first, get_last, set_first, set_last operations.
diff --git a/m4/bison.m4 b/m4/bison.m4
index 5802a5ca5..bda34777b 100644
--- a/m4/bison.m4
+++ b/m4/bison.m4
@@ -1,4 +1,4 @@
-# serial 8
+# serial 9
 
 # Copyright (C) 2002-2006, 2008-2020 Free Software Foundation, Inc.
 # This file is free software; the Free Software Foundation
@@ -25,9 +25,9 @@
 # - %define api.pure   requires bison 2.7 or newer,
 # - %precedencerequires bison 3.0 or newer.
 #   For these, in the configure.ac you will need an invocation of
-# gl_PROG_BISON([VARIABLE], [MIN_BISON_VERSION], 
[BISON_VERSIONS_TO_EXCLUDE])
+# gl_PROG_BISON([VARIABLE], [MIN_BISON_VERSION])
 #   Example:
-# gl_PROG_BISON([PARSE_DATETIME_BISON], [2.4], [1.* | 2.[0-3] | 2.[0-3].*])
+# gl_PROG_BISON([PARSE_DATETIME_BISON], [2.4])
 #   With this preparation, in the Makefile.am there are two ways to formulate
 #   the invocation. Both are direct, without use of 'ylwrap'.
 #   (a) You can invoke
@@ -47,18 +47,23 @@ AC_DEFUN([gl_PROG_BISON],
   if test -z "$[$1]"; then
 ac_verc_fail=yes
   else
-dnl Found it, now check the version.
-AC_MSG_CHECKING([version of bison])
-changequote(<<,>>)dnl
-ac_prog_version=`$<<$1>> --version 2>&1 | sed -n 's/^.*GNU Bison.* 
\([0-9]*\.[0-9.]*\).*$/\1/p'`
-case $ac_prog_version in
-  '') ac_prog_version="v. ?.??, bad"; ac_verc_fail=yes;;
-  <<$3>>)
- ac_prog_version="$ac_prog_version, bad"; ac_verc_fail=yes;;
-  *) ac_prog_version="$ac_prog_version, ok"; ac_verc_fail=no;;
-esac
-changequote([,])dnl
+cat >conftest.y <<_ACEOF
+%require "$2"
+%%
+exp:
+_ACEOF
+AC_MSG_CHECKING([for bison $2 or newer])
+ac_prog_version=`$$1 --version 2>&1 | sed -n 's/^.*GNU Bison.* 
\([[0-9]]*\.[[0-9.]]*\).*$/\1/p'`
+: ${ac_prog_version:='v. ?.??'}
+if _AC_DO_VAR([$1 conftest.y -o conftest.c]); then
+  ac_prog_version="$ac_prog_version, ok"
+  ac_verc_fail=false
+else
+  ac_prog_version="$ac_prog_version, bad"
+  ac_verc_fail=true
+fi
 AC_MSG_RESULT([$ac_prog_version])
+rm -f conftest.y conftest.c
   fi
   if test $ac_verc_fail = yes; then
 [$1]=:
diff --git a/m4/parse-datetime.m4 b/m4/parse-datetime.m4
index 0849b8203..3bb487f9b 100644
--- a/m4/parse-datetime.m4
+++ b/m4/parse-datetime.m4
@@ -35,7 +35,7 @@ AC_DEFUN([gl_PARSE_DATETIME],
   dnl the files or have a broken "make" program, hence the parse-datetime.c
   dnl rule will sometimes fire. To avoid an error, defines PARSE_DATETIME_BISON
   dnl to ":" if it is not 

Re: bison: new module

[Akim Demaille]

Hi Bruno,

> Le 2 mai 2020 à 10:45, Bruno Haible  a écrit :
> 
> Hi Akim,
> 
>> I was suggesting that the macro _itself_ could just
>> run bison on a file with the %require.
> 
> This sounds better than the existing code that runs '--version'.
> 
> What about users who have non-released versions installed?
> I think the %require solution will be on par with the --version
> solution, right?

Yep.  Actually, it would be even more correct, as sometimes
we force the betas (such as 3.5.90) to behave as the release (it
does pass the %require "3.6" requirement).  Which is what I would
expect from the macro.

> What about competing programs (like e.g. clang claims compatibility
> with gcc)? Do competing programs for bison exist and need to be
> worried about?

I know of no program that would be even close to match bison
on its input syntax.  Except of course if you restrict yourself
to the Yacc subset, but then specifying a version requirement
would make no sense: the POSIX Yacc spec has not changed in years.

> At configure time, both solutions are nearly on par: the time to
> create a temporary file is negligible. And both will fail the same
> way for a 'bison' program that references missing shared libraries.
> 
> Patch welcome!

Will do.

Re: m4/bison-i18n.m4: add --with-bisonlocaledir to assign BISON_LOCALEDIR

[Akim Demaille]

Bruno,

Thanks for this!

> Le 2 mai 2020 à 17:31, Bruno Haible  a écrit :
> 
> +BISON_LOCALEDIR="${localedir}"
> +AC_ARG_WITH([bison-prefix],
> +  [[  with-bison-prefix=DIR  search for bison's runtime data in 
> DIR/share]],

You meant --with-bison-prefix here.  Or use AS_HELP_STRING.

Re: wbswidth: portability to cygwin

[Akim Demaille]

Dear gnulibers,

Denis reported a failure in Bison's test suite which is related to the handling 
of mb chars on Cygwin (https://lists.gnu.org/r/bug-bison/2020-05/msg3.html).

Bruno fixed a similar issue (on the same test case actually) on Solaris some 
time ago, and demonstrated it with the following program.  Please find below 
the result on Denis' machine.  The part with wcwidth is probably useless, as 
the results are certainly coming from the degradation of the values of the 
large integral numbers.

I have no idea what are the next steps.  AFAICT from the gnulib documentation, 
wchar.h's replacement should work properly on Cygwin, but I don't know if 
mbswidth is expected to work.  The test Denis ran below was indeed using 
gnulib's wchar.h.

Cheers!

> Le 2 mai 2020 à 12:04, Denis Excoffier  a écrit :
> 
>> On 2020-05-02 07:23, Akim Demaille wrote:
>> 
>> #include 
>> #include 
>> #include 
>> #include 
>> #include "mbswidth.h"
>> int main ()
>> {
>> setlocale (LC_ALL, "en_US.UTF-8");
>> printf ("%d\n", (int) mbswidth ("{∇⃗×퐸⃗ = -∂퐵⃗/∂t}",0)); // 14 vs 17
>> printf ("%d\n", wcwidth (0x2207)); // 1 vs. 2
>> printf ("%d\n", wcwidth (0x20D7)); // 0
>> printf ("%d\n", wcwidth (0x00D7)); // 1
>> printf ("%d\n", wcwidth (0x1D438)); // 1
>> printf ("%d\n", wcwidth (0x2202)); // 1 vs. 2
>> printf ("%d\n", wcwidth (0x1D435)); // 1
>> }
>> 
>> I suggest that you put it in your bison directory, and compile it this way:
>> 
>>> $ gcc foo.c -I _build/g9d/lib -I lib -L _build/g9d/lib -lbison
>> 
>> 
>> (_build/g9d is my build tree)
>> 
>> FTR, I have:
>> 
>>> $ ./a.out
>>> 14
>>> 1
>>> 0
>>> 1
>>> 1
>>> 1
>>> 1
>> 
> 
> On cygwin:
> 
> % uname -sro
> CYGWIN_NT-10.0-17763 3.1.5-340.x86_64.snap Cygwin
> % gcc --version
> gcc (GCC) 9.3.0
> Copyright (C) 2019 Free Software Foundation, Inc.
> This is free software; see the source for copying conditions.  There is NO
> warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
> 
> % gcc -o foo1 foo1.c -I /tmp/lcl/tmp/bison/bison-3.5.91 -I lib -L 
> /tmp/lcl/tmp/bison/bison-3.5.91 -L lib -l bison
> foo1.c: In function 'main':
> foo1.c:13:27: warning: unsigned conversion from 'int' to 'wchar_t’ {aka 
> ‘const short unsigned int’} changes value from '119864' to '54328' 
> [-Woverflow]
>   13 |  printf ("%d\n", wcwidth (0x1D438)); // 1
>  |   ^~~
> foo1.c:15:27: warning: unsigned conversion from 'int' to 'wchar_t’ {aka 
> ‘const short unsigned int’} changes value from '119861' to '54325' 
> [-Woverflow]
>   15 |  printf ("%d\n", wcwidth (0x1D435)); // 1
>  |   ^~~
> % ./foo1
> 16
> 1
> 0
> 1
> 2
> 1
> 2
> %
> 
> Hope this helps,
> 
> Denis Excoffier.

m4/bison-i18n.m4: add --with-bisonlocaledir to assign BISON_LOCALEDIR

[Akim Demaille]

Hi,

Someone suggested that it would be nice to force a given value
to BISON_LOCALEDIR in ./configure.  Are you interested in that?
I'm not a huge fan of adding tons of options to configure, I would
rather have this macro take incoming values of $BISON_LOCALEDIR,
and of course document it with AC_ARG_VAR.

I can wrap a commit if considered relevant.

Cheers!


> Début du message réexpédié :
> 
> De: Hongxu Jia 
> Objet: [PATCH] m4/bison-i18n.m4: add --with-bisonlocaledir to assign 
> BISON_LOCALEDIR
> Date: 18 février 2016 à 07:00:09 UTC+1
> À: 
> 
> The variable BISON_LOCALEDIR is assigned only by the output of
> 'bison --print-localedir', we add option --with-bisonlocaledir
> to assign it explicitly. It is helpful for user to split the
> native compile and cross compile.
> 
> For backward compatibility, if option not used, it still
> make use of the output of 'bison --print-localedir'.
> 
> Signed-off-by: Hongxu Jia 
> ---
> m4/bison-i18n.m4 | 10 --
> 1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/m4/bison-i18n.m4 b/m4/bison-i18n.m4
> index 8e57caf..f74187a 100644
> --- a/m4/bison-i18n.m4
> +++ b/m4/bison-i18n.m4
> @@ -16,11 +16,16 @@ dnl sets BISON_LOCALEDIR to indicate where to find the 
> bison-runtime.mo files
> dnl and defines YYENABLE_NLS if there are bison-runtime.mo files at all.
> AC_DEFUN([BISON_I18N],
> [
> +  dnl Default is not to set bisonlocaledir
> +  AC_ARG_WITH([bisonlocaledir],
> +[  --with-bisonlocaledir   sets BISON_LOCALEDIR to indicate where to 
> find the bison-runtime.mo files],
> +BISON_LOCALEDIR=$withval,
> +BISON_LOCALEDIR=)
> +
>   if test -z "$USE_NLS"; then
> echo "The BISON-I18N macro is used without being preceded by 
> AM-GNU-GETTEXT." 1>&2
> exit 1
>   fi
> -  BISON_LOCALEDIR=
>   BISON_USE_NLS=no
>   if test "$USE_NLS" = yes; then
> dnl Determine bison's localedir.
> @@ -28,9 +33,10 @@ AC_DEFUN([BISON_I18N],
> dnl But even is YACC is called "yacc", it may be a script that invokes 
> bison
> dnl and accepts the --print-localedir option.
> dnl YACC's default value is empty; BISON's default value is :.
> -if (${YACC-${BISON-:}} --print-localedir) >/dev/null 2>&1; then
> +if test -z "$BISON_LOCALEDIR" -a ${YACC-${BISON-:}} --print-localedir 
> >/dev/null 2>&1; then
>   BISON_LOCALEDIR=`${YACC-${BISON-:}} --print-localedir`
> fi
> +AC_MSG_RESULT([$BISON_LOCALEDIR])
> AC_SUBST([BISON_LOCALEDIR])
> if test -n "$BISON_LOCALEDIR"; then
>   dnl There is no need to enable internationalization if the user doesn't
> -- 
> 1.9.1
> 
> 

Re: bison: new module

[Akim Demaille]

Hi Bruno,

> Le 1 mai 2020 à 20:10, Bruno Haible  a écrit :
> 
> Hi Akim,
> 
>> I would personally just run bison on a file with %require "VERSION".
>> It was introduced in 2.2 (2006-05-19), which, I think, is old enough.
>> If you agree with that approach, I can provide a patch.
> 
> Alas, this is not what we want. [1] says
>  "If the requirement is not met, bison exits with an error (exit status 63)."

Sorry, I was very unclear :(  I was answering in the context of the
Autoconf macro: I was suggesting that the macro _itself_ could just
run bison on a file with the %require.

Sorry about that.

Re: bison: new module

[Akim Demaille]

Hi Bruno,

> Le 1 mai 2020 à 18:21, Bruno Haible  a écrit :
> 
> The 'parse-datetime' module has a reasonable configure check for Bison,
> specifying the minimum version of Bison that is required for the particular
> .y file.
> 
> This patch moves the support to a module 'bison', that can be used in other
> gnulib modules or GNU packages as well. It is defined based on
> .
> 
> The macro gl_PROG_BISON could be renamed to AC_PROG_BISON when incorporated
> in GNU Autoconf.

I would personally just run bison on a file with %require "VERSION".
It was introduced in 2.2 (2006-05-19), which, I think, is old enough.
If you agree with that approach, I can provide a patch.

Cheers!

Re: can't bootstrap bison

[Akim Demaille]

Hi Bruno,

> Le 13 avr. 2020 à 16:45, Bruno Haible  a écrit :
> 
> Indeed, 'git submodule update --init' fixes the situation, that
> 'git submodule init' could not fix:

Good!  I installed the following commit.  Cheers!

commit 64e1ff019f9318c63da67d5396ac49a0df53437c
Author: Akim Demaille 
Date:   Mon Apr 13 17:39:38 2020 +0200

bootstrap: recommend git submodule update --init

Reported by Bruno Haible.
<https://lists.gnu.org/r/bug-gnulib/2020-03/msg00101.html>

* build-aux/bootstrap: recommand "git submodule update --init"
rather than "git submodule init".

diff --git a/ChangeLog b/ChangeLog
index 25ce26912..a107fd5b6 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@
+2020-04-13  Akim Demaille  
+
+   bootstrap: recommend git submodule update --init
+   Reported by Bruno Haible.
+   <https://lists.gnu.org/r/bug-gnulib/2020-03/msg00101.html>
+   * build-aux/bootstrap: recommand "git submodule update --init"
+   rather than "git submodule init".
+
 2020-04-12  Bruno Haible  
 
explicit_bzero: Add tests.
diff --git a/build-aux/bootstrap b/build-aux/bootstrap
index 70fd73cc7..8f76d6962 100755
--- a/build-aux/bootstrap
+++ b/build-aux/bootstrap
@@ -1,6 +1,6 @@
 #! /bin/sh
 # Print a version string.
-scriptversion=2019-01-04.17; # UTC
+scriptversion=2020-04-13.15; # UTC
 
 # Bootstrap this package from checked-out sources.
 
@@ -970,7 +970,7 @@ bootstrap_post_import_hook \
 # Uninitialized submodules are listed with an initial dash.
 if $use_git && git submodule | grep '^-' >/dev/null; then
   die "some git submodules are not initialized. " \
-  "Run 'git submodule init' and bootstrap again."
+  "Run 'git submodule update --init' and bootstrap again."
 fi
 
 # Remove any dangling symlink matching "*.m4" or "*.[ch]" in some

Re: can't bootstrap bison

[Akim Demaille]

Hi Bruno,

> Le 7 avr. 2020 à 10:52, Bruno Haible  a écrit :
> 
> Hi Akim,
> 
>>  $ git submodule init
>>  $ ./bootstrap
>>  ...
>>  ./bootstrap: some git submodules are not initialized.  Run 'git submodule 
>> init' and bootstrap again.
>>  $ echo $?
>>  1
> 
> More details:
> 
> $ git -C submodules/autoconf status
> On branch master
> Your branch is up-to-date with 'origin/master'.
> nothing to commit, working directory clean

I don't have that with your tarball.

$ git -C submodules/autoconf status
error: le répertoire objet 
/media/develdata/devel/GNULIB/gnulib-git/.git/objects n'existe pas ; vérifiez 
.git/objects/info/alternates
fatal: bad object HEAD
fatal: 'git status --porcelain=2' a échoué dans le sous-module gnulib

$ git submodule init
$ git -C submodules/autoconf status
error: le répertoire objet 
/media/develdata/devel/GNULIB/gnulib-git/.git/objects n'existe pas ; vérifiez 
.git/objects/info/alternates
fatal: bad object HEAD
fatal: 'git status --porcelain=2' a échoué dans le sous-module gnulib

so AFAICT, git submodule init is unable to init a submodule.  Or at least not 
in the sense I would give to "init".

I think the fix is to have bootstrap say

>> Run 'git submodule update --init' and bootstrap again.

instead.

WDYT?

Re: fstrcmp: memory is not reclaimed on exit

[Akim Demaille]

Hi Bruno,

> Le 16 févr. 2020 à 13:52, Bruno Haible  a écrit :
> 
> Hi Akim,
> 
> Sorry for the delay.

You look busy :)


>> +void
>> +fstrcmp_free (void)
>> +{
>> +  gl_once (keys_init_once, keys_init);
>> +  gl_tls_key_destroy (buffer_key);
>> +  gl_tls_key_destroy (bufmax_key);
>> +}
> 
> This workaround is insufficient, since POSIX [2] says that
>   "It is the responsibility of the application to free any application
>storage or perform any cleanup actions for data structures related
>to the deleted key or associated thread-specific data in any threads"
> 
> In other words, pthread_key_delete is not guaranteed to call the destructor
> of 'buffer_key'. The gnulib test (tests/test-tls.c functions 
> test_tls_dtorcheck1
> and test_tls_dtorcheck2) verifies that the destructor gets called, but only
> for threads != main thread, and you are interested in the main thread
> particularly. Most likely, in this test, the destructor gets called when the
> thread exits [3], not when pthread_key_delete gets called.

Thanks for the details.  The main thread is really not like the others.

> This patch, however, should work.
> 
> 
> 2020-02-16  Bruno Haible  
> 
>   fstrcmp: Add API to clean up resources.
>   Reported by Akim Demaille  in
>   <https://lists.gnu.org/archive/html/bug-gnulib/2020-01/msg00080.html>.
>   * lib/fstrcmp.h (fstrcmp_free_resources): New declaration.
>   * lib/fstrcmp.c (fstrcmp_free_resources): New function.

It looks good to me, thanks!

Re: fstrcmp: memory is not reclaimed on exit

[Akim Demaille]

Hi all,

> Le 22 janv. 2020 à 07:50, Akim Demaille  a écrit :
> 
> I agree, I would like to be able to explicitly release the memory.  But
> I can't see any API to do that in fstrcmp.c.  Is this one ok?  I feel
> stupid to initialize the memory right before releasing, but I didn't
> find a means to check whether the tls memory was initialized.
> 
> Thanks!
> 
> commit eee1a395a841f7d1ae4388710c88c5dd3e047cc0
> Author: Akim Demaille 
> Date:   Wed Jan 22 07:46:45 2020 +0100
> 
>fstrcmp: provide a means to explictly release resources
> 
>* lib/fstrcmp.h, lib/fstrcmp.c (fstrcmp_free): New.

Do we have a better alternative to address this issue?  Is it ok to install?

Cheers!

Re: fstrcmp: memory is not reclaimed on exit

[Akim Demaille]

Hi Bruno,

> Le 20 janv. 2020 à 22:57, Bruno Haible  a écrit :
> 
>>> Initialization:gl_tls_key_init (name, destructor);
>>> 
>>>   A destructor is a function pointer of type 'void (*) (void *)', called
>>>   when a thread exits, and taking the last per-thread value as argument.  It
>>>   is unspecified whether the destructor function is called when the last
>>>   per-thread value is NULL.  On some platforms, the destructor function is
>>>   not called at all.
>> 
>> I can see that it's not expected to work on some platforms, but in the
>> case of the user the platform is fairly mainstream:
> 
> Meanwhile this destructor stuff even works on native Windows. The list of
> platforms where it does not work is very small (something like mingw with
> winpthreads, IIRC).

Great!

>> So I don't know what to do.  Is this a red herring related to Valgrind
>> as a platform?
> 
> Do the threads still exist at the moment valgrind does its inventory of left-
> over memory?

I don't know when it runs its stuff, but I expect, given where it stands,
that it does it as the latest possible instant.

> In particular:
>  - Did you create threads, in which fstrcmp is run? If yes, are they still
>running?

No, Bison does not use any threads.

>  - Or did you run fstrcmp in the main thread? Most likely valgrind does its
>inventory in the main thread, during exit(). This means that at this point
>the fstrcmp buffer for the main thread still exists. In other words, you
>should treat thread-local memory allocations for the main thread like
>static memory allocations (e.g. like in uniqstr.c).

I agree, I would like to be able to explicitly release the memory.  But
I can't see any API to do that in fstrcmp.c.  Is this one ok?  I feel
stupid to initialize the memory right before releasing, but I didn't
find a means to check whether the tls memory was initialized.

Thanks!

commit eee1a395a841f7d1ae4388710c88c5dd3e047cc0
Author: Akim Demaille 
Date:   Wed Jan 22 07:46:45 2020 +0100

fstrcmp: provide a means to explictly release resources

* lib/fstrcmp.h, lib/fstrcmp.c (fstrcmp_free): New.

diff --git a/ChangeLog b/ChangeLog
index 53a1fb435..10626e535 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2020-01-22  Akim Demaille  
+
+   fstrcmp: provide a means to explictly release resources.
+   * lib/fstrcmp.h, lib/fstrcmp.c (fstrcmp_free): New.
+
 2020-01-09  Bruno Haible  
 
c32srtombs: Add tests.
diff --git a/lib/fstrcmp.c b/lib/fstrcmp.c
index c6a66389e..e8e02e856 100644
--- a/lib/fstrcmp.c
+++ b/lib/fstrcmp.c
@@ -73,6 +73,13 @@ keys_init (void)
 /* Ensure that keys_init is called once only.  */
 gl_once_define(static, keys_init_once)
 
+void
+fstrcmp_free (void)
+{
+  gl_once (keys_init_once, keys_init);
+  gl_tls_key_destroy (buffer_key);
+  gl_tls_key_destroy (bufmax_key);
+}
 
 /* In the code below, branch probabilities were measured by Ralf Wildenhues,
by running "msgmerge LL.po coreutils.pot" with msgmerge 0.18 for many
diff --git a/lib/fstrcmp.h b/lib/fstrcmp.h
index 92b67e34a..f3a57ecb6 100644
--- a/lib/fstrcmp.h
+++ b/lib/fstrcmp.h
@@ -38,6 +38,13 @@ extern double fstrcmp_bounded (const char *s1, const char 
*s2,
 /* A shortcut for fstrcmp.  Avoids a function call.  */
 #define fstrcmp(s1,s2) fstrcmp_bounded (s1, s2, 0.0)
 
+/* Explicitly release resources acquired in this thread.  Regular
+   thread termination invokes it automatically, so it is typically not
+   needed to call it.  However, in the case of the main thread, tools
+   such as Valgrind might report "leaks" if the memory is not
+   explicitly reclaimed first.  */
+extern void fstrcmp_free (void);
+
 #ifdef __cplusplus
 }
 #endif

fstrcmp: memory is not reclaimed on exit

[Akim Demaille]

Hi!

There was a recent bug report about Bison not reclaiming all its
memory on exit (https://lists.gnu.org/r/bug-bison/2020-01/msg6.html).
This is a not a leak, the memory is still reachable, yet we try to
stay clean on exit.  The traces from Valgrind are:

> 10. input.at:331: testing Symbol declarations ...
> ./input.at:369: COLUMNS=1000; export COLUMNS;  bison --color=no -fno-caret 
> -Wno-other -S./dump-symbols.m4 input.y
> --- /dev/null 2019-11-30 17:07:08.851263387 +
> +++ 
> /home/tkloczko/rpmbuild/BUILD/bison-3.5/tests/testsuite.dir/at-groups/10/stderr
>2020-01-08 19:14:17.284197849 +
> @@ -0,0 +1,29 @@
> +==2837387== 320 bytes in 1 blocks are still reachable in loss record 1 of 1
> +==2837387==at 0x483A80B: malloc (vg_replace_malloc.c:309)
> +==2837387==by 0x117CFC: xmalloc (xmalloc.c:41)
> +==2837387==by 0x11B743: UnknownInlinedFun (xalloc.h:103)
> +==2837387==by 0x11B743: fstrcmp_bounded (fstrcmp.c:208)
> +==2837387==by 0x142DED: UnknownInlinedFun (symtab.c:350)
> +==2837387==by 0x142DED: UnknownInlinedFun (symtab.c:365)
> +==2837387==by 0x142DED: UnknownInlinedFun (symtab.c:608)
> +==2837387==by 0x142DED: UnknownInlinedFun (symtab.c:1017)
> +==2837387==by 0x142DED: check_and_convert_grammar (reader.c:792)
> +==2837387==by 0x1113E5: UnknownInlinedFun (reader.c:722)
> +==2837387==by 0x1113E5: main (main.c:104)
> +==2837387== 
> +{
> +   
> +   Memcheck:Leak
> +   match-leak-kinds: reachable
> +   fun:malloc
> +   fun:xmalloc
> +   fun:UnknownInlinedFun
> +   fun:fstrcmp_bounded
> +   fun:UnknownInlinedFun
> +   fun:UnknownInlinedFun
> +   fun:UnknownInlinedFun
> +   fun:UnknownInlinedFun
> +   fun:check_and_convert_grammar
> +   fun:UnknownInlinedFun
> +   fun:main
> +}
> 10. input.at:331: 10. Symbol declarations (input.at:331): FAILED 
> (input.at:369)

which points to fstrcmp's allocation of its per-thread internal buffer.

AFAICT, glthread takes care of reclaiming the memory when the thread
exits, that's the whole point of the "free" in the first line of the
body here:

> static void
> keys_init (void)
> {
>   gl_tls_key_init (buffer_key, free);
>   gl_tls_key_init (bufmax_key, NULL);
>   /* The per-thread initial values are NULL and 0, respectively.  */
> }

glthread/tls.h reads

>  Initialization:gl_tls_key_init (name, destructor);
> 
>A destructor is a function pointer of type 'void (*) (void *)', called
>when a thread exits, and taking the last per-thread value as argument.  It
>is unspecified whether the destructor function is called when the last
>per-thread value is NULL.  On some platforms, the destructor function is
>not called at all.

I can see that it's not expected to work on some platforms, but in the
case of the user the platform is fairly mainstream:

> ## - ##
> ## Platform. ##
> ## - ##
> 
> hostname = barrel
> uname -m = x86_64
> uname -r = 5.3.12-300.fc31.x86_64
> uname -s = Linux
> uname -v = #1 SMP Thu Nov 21 22:52:07 UTC 2019

So I don't know what to do.  Is this a red herring related to Valgrind
as a platform?  Is this something else?  I found nothing interesting
about "glthread valgrind" etc. on web, sorry if I missed something.

Thanks!

Re: bug in xtime.h

[Akim Demaille]

Hi friends,

> Le 22 déc. 2019 à 21:48, Paul Eggert  a écrit :
> 
> On 12/22/19 3:31 AM, Bruno Haible wrote:
> 
>> diff --git a/lib/xtime.h b/lib/xtime.h
>> index 77f1c30..5e0ae89 100644
>> --- a/lib/xtime.h
>> +++ b/lib/xtime.h
>> @@ -42,12 +42,13 @@ extern "C" {
>> XTIME_INLINE xtime_t
>> xtime_make (xtime_t s, long int ns)
>> {
>> -  const long int giga = 1000 * 1000 * 1000;
>> -  s += ns / giga;
>> -  ns %= giga;
>>   return XTIME_PRECISION * s + ns;
>> }
> 
> Akim put in that code in October 2018, but I can't see the need for it 
> either. I
> installed the first attached patch to revert that, and am cc'ing this to Akim 
> to
> see why it might be needed. If it is needed, we'll need to complicate the
> overflow suppression a bit.

Well, when I installed it (b159aa5da7e1aa7abeb2f77ba644aa164d25a46d), the diff 
looked like this:

>  /* Return an extended time value that contains S seconds and NS
> -   nanoseconds, without any overflow checking.  */
> +   nanoseconds.  */
>  XTIME_INLINE xtime_t
>  xtime_make (xtime_t s, long int ns)
>  {
> +  const long int giga = 1000 * 1000 * 1000;
> +  s += ns / giga;
> +  ns %= giga;
>if (XTIME_PRECISION == 1)
>  return s;
>else
>  return XTIME_PRECISION * s + ns;
>  }

So the first case (one-second precision) was incorrect.

The commit Bruno refers to (xtime: Assume that the compiler supports 'long 
long') is indeed about that:

> commit 896daf273035e889c50dc5b33e74b5fb891851bc
> Author: Bruno Haible 
> Date:   Sun Dec 22 09:46:46 2019 +0100
> 
> gethrxtime: Assume that the compiler supports 'long long'.
> 
> * lib/xtime.h (xtime_t): Define to 'long long int' always.
> (XTIME_PRECISION): Define to 10 always.
> 
> @@ -57,10 +45,7 @@ xtime_make (xtime_t s, long int ns)
>const long int giga = 1000 * 1000 * 1000;
>s += ns / giga;
>ns %= giga;
> -  if (XTIME_PRECISION == 1)
> -return s;
> -  else
> -return XTIME_PRECISION * s + ns;
> +  return XTIME_PRECISION * s + ns;
>  }

Merry xtime!

maintainer-makefile: update rule for argmatch

[Akim Demaille]

Installed.

commit fffc8765455be047f97e632db7f30e1883c566ad
Author: Akim Demaille 
Date:   Tue Oct 22 10:28:50 2019 +0200

maintainer-makefile: update rule for argmatch

* top/maint.mk (sc_prohibit_argmatch_without_use): Add 
ARGMATCH_DEFINE_GROUP.

diff --git a/ChangeLog b/ChangeLog
index 56102ad45..fc438f503 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2019-10-22  Akim Demaille  
+
+   maintainer-makefile: update rule for argmatch.
+   * top/maint.mk (sc_prohibit_argmatch_without_use): Add 
ARGMATCH_DEFINE_GROUP.
+
 2019-10-21  Akim Demaille  
 
bitset: let freeing functions accept NULL.
diff --git a/top/maint.mk b/top/maint.mk
index 16e936022..b4c2762f0 100644
--- a/top/maint.mk
+++ b/top/maint.mk
@@ -649,7 +649,7 @@ sc_prohibit_safe_read_without_use:
 
 sc_prohibit_argmatch_without_use:
@h='argmatch.h' \
-   
re='(\<(ARRAY_CARDINALITY|X?ARGMATCH(|_TO_ARGUMENT|_VERIFY))\>|\<(invalid_arg|argmatch(_exit_fn|_(in)?valid)?)
 *\()' \
+   
re='(\<(ARGMATCH_DEFINE_GROUP|ARRAY_CARDINALITY|X?ARGMATCH(|_TO_ARGUMENT|_VERIFY))\>|\<(invalid_arg|argmatch(_exit_fn|_(in)?valid)?)
 *\()' \
  $(_sc_header_without_use)
 
 sc_prohibit_canonicalize_without_use:

bitset: let freeing functions accept NULL

[Akim Demaille]

I installed this.  Apparently I was drunk when I wrote the example
in the documentation.

bitsetv does not need these changes, it is already NULL-proof.


commit ac7fd66617c1a3ace838b8660e70930c4182c1e0
Author: Akim Demaille 
Date:   Mon Oct 21 16:47:00 2019 +0200

bitset: let freeing functions accept NULL

* lib/bitset.c (bitset_free, bitset_obstack_free): Do nothing if
given NULL.
* lib/bitset.h: Document that.
* doc/bitset.texi: Fix the example, and demonstrate bitset_free.

diff --git a/ChangeLog b/ChangeLog
index cd6281271..56102ad45 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@
+2019-10-21  Akim Demaille  
+
+   bitset: let freeing functions accept NULL.
+   * lib/bitset.c (bitset_free, bitset_obstack_free): Do nothing if
+   given NULL.
+   * lib/bitset.h: Document that.
+   * doc/bitset.texi: Fix the example, and demonstrate bitset_free.
+
 2019-10-15  Paul Eggert  
 
inttypes: use more-robust test for int range
diff --git a/doc/bitset.texi b/doc/bitset.texi
index d624c0952..b9e540777 100644
--- a/doc/bitset.texi
+++ b/doc/bitset.texi
@@ -48,9 +48,9 @@ Prefer fastest at memory expense.
 enum { nbits = 32 };
 
 bitset bs0 = bitset_create (nbits, BITSET_FIXED);
-bitset_set (bs1, 1);
-bitset_set (bs1, 3);
-bitset_set (bs1, 5);
+bitset_set (bs0, 1);
+bitset_set (bs0, 3);
+bitset_set (bs0, 5);
 
 bitset bs1 = bitset_create (nbits, BITSET_FIXED);
 bitset_set (bs1, 0);
@@ -58,6 +58,10 @@ bitset_set (bs1, 2);
 bitset_set (bs1, 4);
 
 bitset bs = bitset_create (nbits, BITSET_FIXED);
-bitset_or (bs, b1, b2);
+bitset_or (bs, bs0, bs1);
 ASSERT (bitset_count (bs) == 6);
+
+bitset_free (bs);
+bitset_free (bs1);
+bitset_free (bs0);
 @end smallexample
diff --git a/lib/bitset.c b/lib/bitset.c
index 7ea592e13..c3fe923bf 100644
--- a/lib/bitset.c
+++ b/lib/bitset.c
@@ -168,8 +168,11 @@ bitset_create (bitset_bindex n_bits, unsigned attr)
 void
 bitset_free (bitset bset)
 {
-  BITSET_FREE_ (bset);
-  free (bset);
+  if (bset)
+{
+  BITSET_FREE_ (bset);
+  free (bset);
+}
 }
 
 
@@ -177,7 +180,8 @@ bitset_free (bitset bset)
 void
 bitset_obstack_free (bitset bset)
 {
-  BITSET_FREE_ (bset);
+  if (bset)
+BITSET_FREE_ (bset);
 }
 
 
diff --git a/lib/bitset.h b/lib/bitset.h
index e5bb015b8..ea04916ea 100644
--- a/lib/bitset.h
+++ b/lib/bitset.h
@@ -109,7 +109,7 @@ enum bitset_type bitset_type_choose (bitset_bindex, 
bitset_attrs);
 /* Create a bitset of desired type and size.  The bitset is zeroed.  */
 bitset bitset_alloc (bitset_bindex, enum bitset_type);
 
-/* Free bitset.  */
+/* Free bitset.  Do nothing if NULL.  */
 void bitset_free (bitset);
 
 /* Create a bitset of desired type and size using an obstack.  The
@@ -117,7 +117,7 @@ void bitset_free (bitset);
 bitset bitset_obstack_alloc (struct obstack *bobstack,
  bitset_bindex, enum bitset_type);
 
-/* Free bitset allocated on obstack.  */
+/* Free bitset allocated on obstack.  Do nothing if NULL.  */
 void bitset_obstack_free (bitset);
 
 /* Create a bitset of desired size and attributes.  The bitset is zeroed.  */

Re: hash: provide hash_xinitialize

[Akim Demaille]

> Le 9 sept. 2019 à 16:05, Bruno Haible  a écrit :
> 
> Hi Akim,

Hi!

>>* modules/hash (Depends-on): Add xalloc.
>>* lib/hash.h, lib/hash.c (hash_xinitialize): New.
> 
> This patch produces gnulib-tool warnings:
> 
> $ ./gnulib-tool --test hash
> gnulib-tool: warning: module hash depends on a module with an incompatible 
> license: error
> gnulib-tool: warning: module hash depends on a module with an incompatible 
> license: getprogname
> gnulib-tool: warning: module hash depends on a module with an incompatible 
> license: xalloc
> gnulib-tool: warning: module hash depends on a module with an incompatible 
> license: xalloc-die
> ...
> 
> What gnulib-tool is telling you is that the module 'hash' has a license
> that makes it suitable for use in libraries. Adding a dependency to 'xalloc'
> to it can't be done in this module, because library code should never
> call xalloc_die().

Wow!  Nice feature!

Note that the hash module claims LGPLv2+, but the header of hash.[ch] is about 
GPLv3+.

> The fix is to create a new module, with prefix 'x', that adds the
> function. You can leave it declared in hash.h; this is not a problem.
> But it needs to be defined in a different compilation unit.
> 
> For a model of this idiom, look at the modules 'concat-filename'
> and 'xconcat-filename'.


Thanks for the hint; below is my updated proposal (which passes its test :).  I 
see that xconcat-filename is about xconcat_filename; I stayed with 
hash_initialize instead of xhash_initialize, but I can change that, of course.  
Also, contrary to the case of xconcat-filename, I have not put hash.h in the 
Files of xhash, because I see hash.h as a part of hash, not xhash.  And I 
shamelessly decided to put the maintenance burden on Jim's shoulders.


commit 734cd738f062ccbc53126d593e5ae977d77d6ee6
Author: Akim Demaille 
Date:   Mon Sep 9 08:31:33 2019 +0200

xhash: provide hash_xinitialize

Suggested by Egor Pugin 
https://lists.gnu.org/archive/html/bison-patches/2019-09/msg00026.html

* modules/xhash, lib/xhash.c: New.
* lib/hash.h (hash_xinitialize): New.

diff --git a/ChangeLog b/ChangeLog
index 5c226ce43..6969156e3 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@
+2019-09-09  Akim Demaille  
+
+   xhash: provide hash_xinitialize.
+   Suggested by Egor Pugin 
+   https://lists.gnu.org/archive/html/bison-patches/2019-09/msg00026.html
+   * modules/xhash, lib/xhash.c: New.
+   * lib/hash.h (hash_xinitialize): New.
+
 2019-09-06  Akim Demaille  
 
bitset: style changes
diff --git a/lib/hash.h b/lib/hash.h
index a1a483a35..8f2e4591f 100644
--- a/lib/hash.h
+++ b/lib/hash.h
@@ -89,6 +89,9 @@ void hash_reset_tuning (Hash_tuning *);
 Hash_table *hash_initialize (size_t, const Hash_tuning *,
  Hash_hasher, Hash_comparator,
  Hash_data_freer) _GL_ATTRIBUTE_WUR;
+Hash_table *hash_xinitialize (size_t, const Hash_tuning *,
+  Hash_hasher, Hash_comparator,
+  Hash_data_freer) _GL_ATTRIBUTE_WUR;
 void hash_clear (Hash_table *);
 void hash_free (Hash_table *);
 
diff --git a/lib/xhash.c b/lib/xhash.c
new file mode 100644
index 0..9b2bcdbb4
--- /dev/null
+++ b/lib/xhash.c
@@ -0,0 +1,38 @@
+/* hash - hashing table processing.
+
+   Copyright (C) 2019 Free Software Foundation, Inc.
+
+   This program is free software: you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <https://www.gnu.org/licenses/>.  */
+
+#include 
+
+/* Specification.  */
+#include "hash.h"
+
+#include "xalloc.h"
+
+/* Same as hash_initialize, but invokes xalloc_die on memory
+   exhaustion.  */
+
+Hash_table *
+hash_xinitialize (size_t candidate, const Hash_tuning *tuning,
+  Hash_hasher hasher, Hash_comparator comparator,
+  Hash_data_freer data_freer)
+{
+  Hash_table *res =
+hash_initialize (candidate, tuning, hasher, comparator, data_freer);
+  if (!res)
+xalloc_die ();
+  return res;
+}
diff --git a/modules/xhash b/modules/xhash
new file mode 100644
index 0..2e74c718f
--- /dev/null
+++ b/modules/xhash
@@ -0,0 +1,23 @@
+Description:
+Parameterizable hash table, with out-of-memory checking.
+
+Files:
+lib/xhash.c
+
+Depends-on:
+hash
+xalloc
+
+configure.ac:
+
+Makefile.am:
+lib_SOURCES += xhash.c
+
+Include:
+"hash.h"
+
+License:
+GPLv3+
+
+Maintainer:
+Jim Meyering

hash: provide hash_xinitialize

[Akim Demaille]

Hi Jim,

Recently in Bison I had to check all my calls to hash_initialize for memory 
exhaustion.  Coreutils do it by hand for some of the calls, but we could just 
as well provide a simple wrapper?

commit 73f0aa2e58b1dabbe075ab6bf5644da36d7c72d2
Author: Akim Demaille 
Date:   Mon Sep 9 08:31:33 2019 +0200

hash: provide hash_xinitialize.

Suggested by Egor Pugin 
https://lists.gnu.org/archive/html/bison-patches/2019-09/msg00026.html

* modules/hash (Depends-on): Add xalloc.
* lib/hash.h, lib/hash.c (hash_xinitialize): New.

diff --git a/ChangeLog b/ChangeLog
index 5c226ce43..ca12b170b 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@
+2019-09-09  Akim Demaille  
+
+   hash: provide hash_xinitialize.
+   Suggested by Egor Pugin 
+   https://lists.gnu.org/archive/html/bison-patches/2019-09/msg00026.html
+   * modules/hash (Depends-on): Add xalloc.
+   * lib/hash.h, lib/hash.c (hash_xinitialize): New.
+
 2019-09-06  Akim Demaille  
 
bitset: style changes
diff --git a/lib/hash.c b/lib/hash.c
index 9e1f8e841..ca1d04044 100644
--- a/lib/hash.c
+++ b/lib/hash.c
@@ -27,6 +27,7 @@
 #include "hash.h"
 
 #include "bitrotate.h"
+#include "xalloc.h"
 #include "xalloc-oversized.h"
 
 #include 
@@ -645,6 +646,22 @@ hash_initialize (size_t candidate, const Hash_tuning 
*tuning,
   return NULL;
 }
 
+
+/* Same as hash_initialize, but invokes xalloc_die on memory
+   exhaustion.  */
+
+Hash_table *
+hash_xinitialize (size_t candidate, const Hash_tuning *tuning,
+  Hash_hasher hasher, Hash_comparator comparator,
+  Hash_data_freer data_freer)
+{
+  Hash_table *res =
+hash_initialize (candidate, tuning, hasher, comparator, data_freer);
+  if (!res)
+xalloc_die ();
+  return res;
+}
+
 /* Make all buckets empty, placing any chained entries on the free list.
Apply the user-specified function data_freer (if any) to the datas of any
affected entries.  */
diff --git a/lib/hash.h b/lib/hash.h
index a1a483a35..8f2e4591f 100644
--- a/lib/hash.h
+++ b/lib/hash.h
@@ -89,6 +89,9 @@ void hash_reset_tuning (Hash_tuning *);
 Hash_table *hash_initialize (size_t, const Hash_tuning *,
  Hash_hasher, Hash_comparator,
  Hash_data_freer) _GL_ATTRIBUTE_WUR;
+Hash_table *hash_xinitialize (size_t, const Hash_tuning *,
+  Hash_hasher, Hash_comparator,
+  Hash_data_freer) _GL_ATTRIBUTE_WUR;
 void hash_clear (Hash_table *);
 void hash_free (Hash_table *);
 
diff --git a/modules/hash b/modules/hash
index 42502e749..31303c1a8 100644
--- a/modules/hash
+++ b/modules/hash
@@ -9,6 +9,7 @@ Depends-on:
 bitrotate
 stdbool
 stdint
+xalloc
 xalloc-oversized
 
 configure.ac:

Re: bitset: check memory allocation

[Akim Demaille]

> Le 8 sept. 2019 à 03:17, Paul Eggert  a écrit :
> 
> Thanks, and your changes all look good to me; please install when you have 
> the time. We can worry about the other issues later (if ever...).

Installed, thanks!

Re: bitset: check memory allocation

[Akim Demaille]

Hi Paul,

Thanks a lot for the detailed answer!

> Le 5 sept. 2019 à 22:54, Paul Eggert  a écrit :
> 
> On 9/5/19 12:59 PM, Akim Demaille wrote:
> 
>>EBITSET_ELTS (src)
>> -= realloc (EBITSET_ELTS (src), newsize * sizeof (tbitset_elt 
>> *));
>> += xrealloc (EBITSET_ELTS (src), newsize * sizeof (tbitset_elt 
>> *));
> 
> This code is trying to shrink the bitset, and in the unlikely event that such 
> an attempt fails there's no need call xrealloc which will exit the whole 
> program; the code can continue to call realloc and if that fails, the code 
> can forge ahead with the unshrunk bitset.
> 
> There's a similar issue in the vector.c patch.

I never noticed realloc would return 0 yet preserve the allocated region, 
thanks!  This patch should address your concerns:

commit c9c23ad1d50cb5746dc2dd0265e0ae91c746b0b9
Author: Akim Demaille 
Date:   Thu Sep 5 11:36:39 2019 +0200

bitset: check memory allocation

Reported by 江 祖铭 (Zu-Ming Jiang).
With help from Paul Eggert.
https://lists.gnu.org/archive/html/bug-bison/2019-08/msg00016.html

* lib/bitset/table.c (tbitset_resize): When growing, use xrealloc
instead of realloc.
When shrinking, accept failures.
* lib/bitset/vector.c (vbitset_resize): Likewise.

diff --git a/ChangeLog b/ChangeLog
index fbf95966d..08b2313cb 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,14 @@
+2019-09-06  Akim Demaille  
+
+   bitset: check memory allocation
+   Reported by 江 祖铭 (Zu-Ming Jiang).
+   With help from Paul Eggert.
+   https://lists.gnu.org/archive/html/bug-bison/2019-08/msg00016.html
+   * lib/bitset/table.c (tbitset_resize): When growing, use xrealloc
+   instead of realloc.
+   When shrinking, accept failures.
+   * lib/bitset/vector.c (vbitset_resize): Likewise.
+
 2019-09-01  Bruno Haible  
 
gitsub.sh: Add support for shallow-cloning of subdirectories.
diff --git a/lib/bitset/table.c b/lib/bitset/table.c
index 07184d657..7691d9805 100644
--- a/lib/bitset/table.c
+++ b/lib/bitset/table.c
@@ -25,6 +25,7 @@
 #include 
 
 #include "obstack.h"
+#include "xalloc.h"
 
 /* This file implements expandable bitsets.  These bitsets can be of
arbitrary length and are more efficient than arrays of bits for
@@ -142,7 +143,7 @@ tbitset_resize (bitset src, bitset_bindex n_bits)
 
   bitset_windex size = oldsize == 0 ? newsize : newsize + newsize / 4;
   EBITSET_ELTS (src)
-= realloc (EBITSET_ELTS (src), size * sizeof (tbitset_elt *));
+= xrealloc (EBITSET_ELTS (src), size * sizeof (tbitset_elt *));
   EBITSET_ASIZE (src) = size;
 }
 
@@ -155,9 +156,13 @@ tbitset_resize (bitset src, bitset_bindex n_bits)
  the memory unless it is shrinking by a reasonable amount.  */
   if ((oldsize - newsize) >= oldsize / 2)
 {
-  EBITSET_ELTS (src)
+  void *p
 = realloc (EBITSET_ELTS (src), newsize * sizeof (tbitset_elt *));
-  EBITSET_ASIZE (src) = newsize;
+  if (p)
+{
+  EBITSET_ELTS (src) = p;
+  EBITSET_ASIZE (src) = newsize;
+}
 }
 
   /* Need to prune any excess bits.  FIXME.  */
diff --git a/lib/bitset/vector.c b/lib/bitset/vector.c
index 54f148d56..5e543283a 100644
--- a/lib/bitset/vector.c
+++ b/lib/bitset/vector.c
@@ -24,6 +24,8 @@
 #include 
 #include 
 
+#include "xalloc.h"
+
 /* This file implements variable size bitsets stored as a variable
length array of words.  Any unused bits in the last word must be
zero.
@@ -74,7 +76,7 @@ vbitset_resize (bitset src, bitset_bindex n_bits)
 
   bitset_windex size = oldsize == 0 ? newsize : newsize + newsize / 4;
   VBITSET_WORDS (src)
-= realloc (VBITSET_WORDS (src), size * sizeof (bitset_word));
+= xrealloc (VBITSET_WORDS (src), size * sizeof (bitset_word));
   VBITSET_ASIZE (src) = size;
 }
 
@@ -88,9 +90,13 @@ vbitset_resize (bitset src, bitset_bindex n_bits)
  the memory unless it is shrinking by a reasonable amount.  */
   if ((oldsize - newsize) >= oldsize / 2)
 {
-  VBITSET_WORDS (src)
+  void *p
 = realloc (VBITSET_WORDS (src), newsize * sizeof (bitset_word));
-  VBITSET_ASIZE (src) = newsize;
+  if (p)
+{
+  VBITSET_WORDS (src) = p;
+  VBITSET_ASIZE (src) = newsize;
+}
 }
 
   /* Need to prune any excess bits.  FIXME.  */


> Also, a nit in nearby code: xcalloc (1, N) is better written as xzalloc (N).

How about this stylistic patch?

commit 44e2124e57689417228fc04ded0026dc53cbee57
Author: Akim Demaille 
Date:   Fri Sep 6 11:38:48 2019 +0200

bitset: style changes

* lib/bitset/vector.c (vbitset_resize): Factor computation.
* lib/bitset.c,

bitset: check memory allocation

[Akim Demaille]

Hi!

Ok to push?

commit 1fddc3eddd988829cd2b5b74dc3fab58a3093af8
Author: Akim Demaille 
Date:   Thu Sep 5 11:36:39 2019 +0200

bitset: check memory allocation

Reported by 江 祖铭 (Zu-Ming Jiang).
https://lists.gnu.org/archive/html/bug-bison/2019-08/msg00016.html

* lib/bitset/table.c (tbitset_resize): Use xrealloc instead of
realloc.
* lib/bitset/vector.c (vbitset_resize): Likewise.

diff --git a/lib/bitset/table.c b/lib/bitset/table.c
index 07184d657..01bc65167 100644
--- a/lib/bitset/table.c
+++ b/lib/bitset/table.c
@@ -25,6 +25,7 @@
 #include 
 
 #include "obstack.h"
+#include "xalloc.h"
 
 /* This file implements expandable bitsets.  These bitsets can be of
arbitrary length and are more efficient than arrays of bits for
@@ -142,7 +143,7 @@ tbitset_resize (bitset src, bitset_bindex n_bits)
 
   bitset_windex size = oldsize == 0 ? newsize : newsize + newsize / 4;
   EBITSET_ELTS (src)
-= realloc (EBITSET_ELTS (src), size * sizeof (tbitset_elt *));
+= xrealloc (EBITSET_ELTS (src), size * sizeof (tbitset_elt *));
   EBITSET_ASIZE (src) = size;
 }
 
@@ -156,7 +157,7 @@ tbitset_resize (bitset src, bitset_bindex n_bits)
   if ((oldsize - newsize) >= oldsize / 2)
 {
   EBITSET_ELTS (src)
-= realloc (EBITSET_ELTS (src), newsize * sizeof (tbitset_elt *));
+= xrealloc (EBITSET_ELTS (src), newsize * sizeof (tbitset_elt *));
   EBITSET_ASIZE (src) = newsize;
 }
 
diff --git a/lib/bitset/vector.c b/lib/bitset/vector.c
index 54f148d56..11960bbd0 100644
--- a/lib/bitset/vector.c
+++ b/lib/bitset/vector.c
@@ -24,6 +24,8 @@
 #include 
 #include 
 
+#include "xalloc.h"
+
 /* This file implements variable size bitsets stored as a variable
length array of words.  Any unused bits in the last word must be
zero.
@@ -74,7 +76,7 @@ vbitset_resize (bitset src, bitset_bindex n_bits)
 
   bitset_windex size = oldsize == 0 ? newsize : newsize + newsize / 4;
   VBITSET_WORDS (src)
-= realloc (VBITSET_WORDS (src), size * sizeof (bitset_word));
+= xrealloc (VBITSET_WORDS (src), size * sizeof (bitset_word));
   VBITSET_ASIZE (src) = size;
 }
 
@@ -89,7 +91,7 @@ vbitset_resize (bitset src, bitset_bindex n_bits)
   if ((oldsize - newsize) >= oldsize / 2)
 {
   VBITSET_WORDS (src)
-= realloc (VBITSET_WORDS (src), newsize * sizeof (bitset_word));
+= xrealloc (VBITSET_WORDS (src), newsize * sizeof (bitset_word));
   VBITSET_ASIZE (src) = newsize;
 }
 

Re: maintainer-makefile: catch uses of $< in non-implicit rules

[Akim Demaille]

Hi Tim,

Sorry I dropped the ball...

> Reproducible everywhere (needs gawk being installed, else the
> sc_prohibit_gnu_make_extensions is a no-op).

Which is what I meant.  So are you saying it work as (I) expected?

> Akim, at least with GNU make 4.2.1 the combination of -q and -p doesn't
> do what you expect. From the make man page, I would say that both
> options contradict. -q: don't print anything; -p: print the database

I'm using 4.2.1, and it does what I meant: -p prints the rules,
and -q (which is --question, not --quiet) avoids that we
fired a rule (i.e., "make -q" does not run "make all").

So I'm just clueless here.  I don't know what to do to address
your issue.

Are you running "make syntax-check" on a configured builddir?

argmatch: adjust columns for help2man

[Akim Demaille]

Bison's man page included the following incorrect snippet (first four lines are 
not properly indented as a definition list):

>FEATURES is a list of comma separated words that can include:
>   caret, diagnostics-show-caret
> 
>   show errors with carets
> 
>   fixit, diagnostics-parseable-fixits
> 
>   show machine-readable fixes
> 
>syntax-only
>   do not generate any file
> 
>allall of the above
> 
>none   disable all of the above

because --help looked like this:

> FEATURES is a list of comma separated words that can include:
>   caret, diagnostics-show-caret
>show errors with carets
>   fixit, diagnostics-parseable-fixits
>show machine-readable fixes
>   syntax-only  do not generate any file
>   all  all of the above
>   none disable all of the above

But that format is not recognized by help2man.  Its author,
Brendan O'Dea, wrote to me:

> The heuristic help2man uses when mapping input to a tagged paragraph
> is fairly simple: it will match a term and a definition when:
> 
>  a) the term is indented by at least one character  AND
>  b) the definition either:
> i) appears on the same line, separated by two or more spaces  OR
> ii) is on the next line, indented by twenty or more spaces
> 
> Your first example is meeting (a) and (b i), your second fails (b ii).
> The requirement for twenty spaces is a bit arbitrary, but mostly seems
> to work OK, and has less false-positives than smaller indents which
> were experimented with (long ago).

The appended changes (pushed) generate this in bison --help:

> FEATURES is a list of comma separated words that can include:
>   caret, diagnostics-show-caret
> show errors with carets
>   fixit, diagnostics-parseable-fixits
> show machine-readable fixes
>   syntax-only   do not generate any file
>   all   all of the above
>   none  disable all of the above


which gives this in the man page.

>FEATURES is a list of comma separated words that can include:
>caret, diagnostics-show-caret
>   show errors with carets
> 
>fixit, diagnostics-parseable-fixits
>   show machine-readable fixes
> 
>syntax-only
>   do not generate any file
> 
>allall of the above
> 
>none   disable all of the above

Cheers!


commit ee77e5c1fef322b8c0a6596aa9b2c43323eff4d1
Author: Akim Demaille 
Date:   Sun Jul 7 12:12:25 2019 +0200

argmatch: adjust columns for help2man.

* lib/argmatch.h (argmatch_##Name##_doc_col): If some argument
requires column 20 or more, return 20.

diff --git a/ChangeLog b/ChangeLog
index f7e031d9b..01696987f 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2019-07-07  Akim Demaille  
+
+   argmatch: adjust columns for help2man.
+   * lib/argmatch.h (argmatch_##Name##_doc_col): If some argument
+   requires column 20 or more, return 20.
+
 2019-07-06  Paul Eggert  
 
areadlink-with-size: avoid realloc when size==0
diff --git a/lib/argmatch.h b/lib/argmatch.h
index 57d131f32..897fa415d 100644
--- a/lib/argmatch.h
+++ b/lib/argmatch.h
@@ -264,9 +264,8 @@ char const *argmatch_to_argument (void const *value,
   for (int j = 0; g->args[j].arg; ++j)  \
 if (! memcmp (>args[ival].val, >args[j].val, size))   \
   col += (col == 4 ? 0 : 2) + strlen (g->args[j].arg);  \
-/* Ignore series that are too wide. */  \
-if (col <= 20 && res <= col)\
-  res = col;\
+if (res <= col) \
+  res = col <= 20 ? col : 20;   \
   } \
 return res ? res : 20;  \
   } \

Re: argmatch.h: new "_" definition may conflict with that of callers

[Akim Demaille]

Hi Jim!

> Le 3 juil. 2019 à 06:17, Jim Meyering  a écrit :
> 
> Hi Akim,
> I tried to build grep using latest gnulib and it failed with these errors:
> 
>  CC   grep.o
> In file included from grep.c:30:
> ../lib/argmatch.h:35: error: "_" redefined [-Werror]
>   35 | # define _(Msgid)  gettext (Msgid)
>  |
> In file included from grep.c:28:
> system.h:43: note: this is the location of the previous definition
>   43 | #define _(String) gettext(String)
>  |
> In file included from grep.c:30:
> ../lib/argmatch.h:36: error: "N_" redefined [-Werror]
>   36 | # define N_(Msgid) (Msgid)
>  |
> In file included from grep.c:28:
> system.h:42: note: this is the location of the previous definition
>   42 | #define N_(String) gettext_noop(String)
> 
> First, the "N_" definition appears unnecessary: it is used in neither
> of argmatch.[ch].
> One fix for the "_" problem is to expand it: s/\b_\b/gettext /
> 
> Care to fix it, one way or another?

Bummer :(  Sorry about that.  I installed the following commit.

Cheers!

commit 65b3308bb6a8e42b0d5a4550a62562790e10
Author: Akim Demaille 
Date:   Wed Jul 3 06:35:34 2019 +0200

argmatch: don't define _ in the header

Reported by Jim Meyering.

* lib/argmatch.h (N_, _): Don't define.
Use gettext instead.
* lib/argmatch.h (_): Define.
* tests/test-argmatch.c (N_): Define.

diff --git a/ChangeLog b/ChangeLog
index b89fd5d9e..c3e197870 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,12 @@
+2019-07-03  Akim Demaille  
+
+   argmatch: don't define _ in the header.
+   Reported by Jim Meyering.
+   * lib/argmatch.h (N_, _): Don't define.
+   Use gettext instead.
+   * lib/argmatch.h (_): Define.
+   * tests/test-argmatch.c (N_): Define.
+
 2019-07-02  Paul Eggert  
 
verify: document ‘assume’ better
diff --git a/lib/argmatch.c b/lib/argmatch.c
index 183a0ca94..9eeb4514d 100644
--- a/lib/argmatch.c
+++ b/lib/argmatch.c
@@ -29,6 +29,8 @@
 #include 
 #include 
 
+#define _(msgid) gettext (msgid)
+
 #include "error.h"
 #include "quotearg.h"
 #include "getprogname.h"
diff --git a/lib/argmatch.h b/lib/argmatch.h
index d18549942..57d131f32 100644
--- a/lib/argmatch.h
+++ b/lib/argmatch.h
@@ -32,9 +32,6 @@
 # include "quote.h"
 # include "verify.h"
 
-# define _(Msgid)  gettext (Msgid)
-# define N_(Msgid) (Msgid)
-
 # ifdef  __cplusplus
 extern "C" {
 # endif
@@ -223,7 +220,7 @@ char const *argmatch_to_argument (void const *value,
 \
 /* Try to put synonyms on the same line.  Synonyms are expected \
to follow each other. */ \
-fputs (_("Valid arguments are:"), out); \
+fputs (gettext ("Valid arguments are:"), out);  \
 for (int i = 0; g->args[i].arg; i++)\
   if (i == 0\
   || memcmp (>args[i-1].val, >args[i].val, size)) \
@@ -284,7 +281,7 @@ char const *argmatch_to_argument (void const *value,
large width. */  \
 const int screen_width = getenv ("HELP2MAN") ? INT_MAX : 80;\
 if (g->doc_pre) \
-  fprintf (out, "%s\n", _(g->doc_pre)); \
+  fprintf (out, "%s\n", gettext (g->doc_pre));  \
 int doc_col = argmatch_##Name##_doc_col (); \
 for (int i = 0; g->docs[i].arg; ++i)\
   { \
@@ -321,10 +318,11 @@ char const *argmatch_to_argument (void const *value,
 fprintf (out, "\n");\
 col = 0;\
   } \
-fprintf (out, "%*s%s\n", doc_col - col, "", _(g->docs[i].doc)); \
+fprintf (out, "%*s%s\n",\
+ doc_col - col, "", gettext (g->docs[i].doc));  \
   } \
 if (g->doc_post)\
-  fprintf (out, "%s\n", _(g->doc_post));\
+  fprintf (out, "%s\n", gettext (g->doc_post)); \
   }
 
 # ifdef  __cplusplus
diff --git a/tests/test-argmatch.c b/tests/test-argmatch.c
index 11b5819ea..7b1b38a19 100644
--- a/tests/test-argmatch.c
+++ b/tests/test-argmatch.c
@@ -25,6 +25,8 @@
 
 #include "macros.h"
 
+# define N_(Msgid) (Msgid)
+
 /* Some packages define ARGMATCH_DIE and ARGMATCH_DIE_DECL in , and
thus must link with a definition of that function.  Provide it here.  */
 #ifdef ARGMATCH_DIE_DECL