[PATCH v2 0/6] A handful of post 3.0.10 fixups

[PATCH v2 0/6] A handful of post 3.0.10 fixups

[Rob Browning]

I generated this set of patches while trying to track down the Debian
buildd failures.  The first four are bug fixes, and the other two are
an optimization and a doc fix.

Proposed for main, and if they're deemed plausible, I'll add any
relevant NEWS updates.

The main change as compared to v2 is to the patch, now "Ensure tests
have guile-procedures.txt" that previously proposed adding it to
BUILT_SOURCES.  That wasn't quite right.

Thanks

Rob Browning (6):
  Ensure GUILE-VERSION changes propagate to .version and Makefiles
  Ensure tests have guile-procedures.txt
  test-hashing: support 32-bit
  scm_i_utf8_string_hash: don't overrun when len is zero
  scm_i_utf8_string_hash: optimize ASCII
  define-meta-command: mention effects of a missing category

 Makefile.am                          | 24 +++++-------
 configure.ac                         |  1 +
 libguile/Makefile.am                 | 15 +++++---
 libguile/hash.c                      | 56 +++++++++++++++-------------
 module/system/repl/command.scm       |  2 +
 test-suite/standalone/test-hashing.c |  8 +++-
 6 files changed, 59 insertions(+), 47 deletions(-)

-- 
2.43.0

[PATCH v2 1/6] Ensure GUILE-VERSION changes propagate to .version and Makefiles

[Rob Browning]

Have .version depend on the Makefile, and move our
CONFIG_STATUS_DEPENDENCIES setting to an AC_SUBST, as recommended by the
automake info pages "Rebuilding Makefiles" section, so that changes to
GUILE-VERSION will update the VERSION, etc. in the generated Makefiles.

* Makefile.am (CONFIG_STATUS_DEPENDENCIES): drop.
($(top_srcdir/.version)): depend on Makefile.
* configure: add GUILE-VERSION to CONFIG_STATUS_DEPENDENCIES via
AC_SUBST.
---
 Makefile.am  | 4 +---
 configure.ac | 1 +
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index b2ac5539e..c74761628 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -110,8 +110,6 @@ endif !HAVE_READLINE
 clean-local:
 	rm -rf cache/
 
-CONFIG_STATUS_DEPENDENCIES = GUILE-VERSION
-
 gen_start_rev = 61db429e251bfd2f75cb4632972e0238056eb24b
 .PHONY: gen-ChangeLog
 gen-ChangeLog:
@@ -133,7 +131,7 @@ assert-no-store-file-names:
 	fi
 
 BUILT_SOURCES += $(top_srcdir)/.version
-$(top_srcdir)/.version:
+$(top_srcdir)/.version: Makefile
 	echo $(VERSION) > $@-t && mv $@-t $@
 gen-tarball-version:
 	echo $(VERSION) > $(distdir)/.tarball-version
diff --git a/configure.ac b/configure.ac
index 0dcb71cce..262b171f1 100644
--- a/configure.ac
+++ b/configure.ac
@@ -40,6 +40,7 @@ m4_ifdef([AM_SILENT_RULES], [AM_SILENT_RULES([yes])], [AC_SUBST([AM_DEFAULT_VERB
 AC_COPYRIGHT(GUILE_CONFIGURE_COPYRIGHT)
 AC_CONFIG_SRCDIR([GUILE-VERSION])
 
+AC_SUBST([CONFIG_STATUS_DEPENDENCIES], ['$(top_srcdir)/GUILE-VERSION'])
 . $srcdir/GUILE-VERSION
 GUILE_VERSION="$PACKAGE_VERSION"
 
-- 
2.43.0

[PATCH v2 2/6] Ensure tests have guile-procedures.txt

[Rob Browning]

The tests depend on libguile/guile-procedures.txt, for example via
documented? in bit-operations.test.  Previously "make check -j..." in a
clean tree would fail because libguile/guile-procedures.txt is built by
./Makefile.am (rather than libguile/Makefile.am) so that it will have a
built module/ available, but when "." is not listed in SUBDIRS, it
builds last, and so the test-suite runs before guile-procedures.txt is
built.

To fix the problem add "." to SUBDIRS before the test-suite so that the
tests will be able depend on everything else, and move the existing
guile-procedures.txt target into libguile/ next to its
guile-procedures.texi dependency.  That gives a better overview and
simplifies the recipe a bit.  It also allows us to drop the explict
"all-local:" dependency, and to let the existing libguile/ code handle
the cleanup.

* Makefile.am (SUBDIRS): add . just before the test-suite.
(libguile/guile-procedures.txt): rely on libguile/Makefile.am.
(CLEANFILES): Drop libguile/procedures.txt.
* libguile/Makefile.am: (all-local): drop.
(libguile/guile-procedures.txt): move Makefile.am recipe here.
---
 Makefile.am          | 20 ++++++++------------
 libguile/Makefile.am | 15 +++++++++------
 2 files changed, 17 insertions(+), 18 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index c74761628..6d73625c7 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -26,6 +26,8 @@
 #
 AUTOMAKE_OPTIONS = 1.10
 
+# The test-suite goes last so that it can depend on everything else
+# (e.g. guile-procedures.txt).
 SUBDIRS =					\
 	lib					\
 	meta					\
@@ -36,8 +38,9 @@ SUBDIRS =					\
 	stage2					\
 	guile-readline				\
 	examples				\
-	test-suite				\
-	doc
+	doc					\
+	.					\
+	test-suite
 
 DIST_SUBDIRS = $(SUBDIRS) prebuilt
 
@@ -57,15 +60,9 @@ BUILT_SOURCES = libguile/scmconfig.h
 libguile/scmconfig.h:
 	$(MAKE) -C libguile scmconfig.h
 
-# Build it from here so that all the modules are compiled by the time we
-# build it.
-libguile/guile-procedures.txt: libguile/guile-procedures.texi
-	$(AM_V_GEN)						\
-	$(top_builddir)/meta/guile --no-auto-compile		\
-	  "$(srcdir)/libguile/texi-fragments-to-docstrings"	\
-	  "$(builddir)/libguile/guile-procedures.texi"		\
-	  > $@.tmp
-	@mv $@.tmp $@
+# Build it here so that (given SUBDIRS order) the modules are available
+libguile/guile-procedures.txt:
+	$(MAKE) -C libguile guile-procedures.txt
 
 EXTRA_DIST = LICENSE HACKING GUILE-VERSION			\
 	     am/maintainer-dirs					\
@@ -87,7 +84,6 @@ EXTRA_DIST = LICENSE HACKING GUILE-VERSION			\
 
 ACLOCAL_AMFLAGS = -I m4
 
-CLEANFILES = libguile/guile-procedures.txt
 DISTCLEANFILES = check-guile.log
 
 DISTCHECK_CONFIGURE_FLAGS = --enable-error-on-warning --enable-mini-gmp
diff --git a/libguile/Makefile.am b/libguile/Makefile.am
index 8d8fa27d7..3ea2e57da 100644
--- a/libguile/Makefile.am
+++ b/libguile/Makefile.am
@@ -472,10 +472,6 @@ BUILT_INCLUDES = vm-operations.h scmconfig.h libpath.h srfi-14.i.c
 BUILT_SOURCES = cpp-E.c cpp-SIG.c $(BUILT_INCLUDES) \
     $(DOT_X_FILES) $(EXTRA_DOT_X_FILES)
 
-# Force the generation of `guile-procedures.texi' because the top-level
-# Makefile expects it to be built.
-all-local: guile-procedures.texi
-
 EXTRA_libguile_@GUILE_EFFECTIVE_VERSION@_la_SOURCES = \
     syscalls.h					\
     dynl.c regex-posix.c			\
@@ -739,8 +735,6 @@ EXTRA_DIST = ChangeLog-scm ChangeLog-threads				\
     unidata_to_charset.awk UnicodeData.txt				\
     vm-operations.h libguile-@GUILE_EFFECTIVE_VERSION@-gdb.scm		\
     $(lightening_c_files) $(lightening_extra_files)
-#    $(DOT_DOC_FILES) $(EXTRA_DOT_DOC_FILES) \
-#    guile-procedures.txt guile.texi
 
 ## FIXME: Consider using timestamp file, to avoid unnecessary rebuilds.
 if MINGW_LIBPATH
@@ -858,6 +852,15 @@ guile.texi: $(alldotdocfiles) guile$(EXEEXT)
 guile-procedures.texi: $(alldotdocfiles) guile$(EXEEXT)
 	$(AM_V_GEN)$(dotdoc2texi)          > $@ || { rm $@; false; }
 
+# Requires meta to be listed before libguile in SUBDIRS
+guile-procedures.txt: guile-procedures.texi
+	$(AM_V_GEN)						\
+	$(top_builddir)/meta/guile --no-auto-compile		\
+	  "$(srcdir)/texi-fragments-to-docstrings"		\
+	  "$(builddir)/guile-procedures.texi"			\
+	  > $@.tmp
+	@mv $@.tmp $@
+
 c-tokenize.c: c-tokenize.lex
 	flex -t $(srcdir)/c-tokenize.lex > $@ || { rm $@; false; }
 
-- 
2.43.0

[PATCH v2 3/6] test-hashing: support 32-bit

[Rob Browning]

* test-suite/standalone/test-hashing.c (test_hashing): add expected
value for 32-bit architectures.
---
 test-suite/standalone/test-hashing.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/test-suite/standalone/test-hashing.c b/test-suite/standalone/test-hashing.c
index 5982a0fdb..50e132989 100644
--- a/test-suite/standalone/test-hashing.c
+++ b/test-suite/standalone/test-hashing.c
@@ -38,9 +38,15 @@ test_hashing ()
 
   // Value determined by calling wide_string_hash on {0x3A0, 0x3B5,
   // 0x3C1, 0x3AF} via a temporary test program.
+#if SIZEOF_UNSIGNED_LONG == 8
   const unsigned long expect = 4029223418961680680;
-  const unsigned long actual = scm_to_ulong (scm_symbol_hash (sym));
+#elif SIZEOF_UNSIGNED_LONG == 4
+  const unsigned long expect = 938126682;
+#else
+#error "unsigned long not 4 or 8 bytes (need additonal test data)"
+#endif
 
+  const unsigned long actual = scm_to_ulong (scm_symbol_hash (sym));
   if (actual != expect)
     {
       fprintf (stderr, "fail: unexpected utf-8 symbol hash (%lu != %lu)\n",
-- 
2.43.0

Re: [PATCH v2 3/6] test-hashing: support 32-bit

[Rob Browning]

Rob Browning <rlb@defaultvalue.org> writes:

> * test-suite/standalone/test-hashing.c (test_hashing): add expected
> value for 32-bit architectures.

Pushed this one to main.

-- 
Rob Browning
rlb @defaultvalue.org and @debian.org
GPG as of 2011-07-10 E6A9 DA3C C9FD 1FF8 C676 D2C4 C0F0 39E9 ED1B 597A
GPG as of 2002-11-03 14DD 432F AE39 534D B592 F9A0 25C8 D377 8C7E 73A4

[PATCH v2 4/6] scm_i_utf8_string_hash: don't overrun when len is zero

[Rob Browning]

When the length is zero, the previous code would include the byte after
the end of the string in the hash.  Fix that (the wide an narrow hashers
also guard against it via "case 0"), and while we're there, switch to
u8_mbtouc since the unsafe variant is now the same (see the info pages),
and don't bother mutating length for the trailing bytes, since we don't
need to.

libguile/hash.c (scm_i_utf8_string_hash): switch to u8_mbtouc, and avoid
overrun when len == 0.
---
 libguile/hash.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/libguile/hash.c b/libguile/hash.c
index a038a11bf..d92f60df8 100644
--- a/libguile/hash.c
+++ b/libguile/hash.c
@@ -195,32 +195,34 @@ scm_i_utf8_string_hash (const char *str, size_t len)
   /* Handle most of the key.  */
   while (length > 3)
     {
-      ustr += u8_mbtouc_unsafe (&u32, ustr, end - ustr);
+      ustr += u8_mbtouc (&u32, ustr, end - ustr);
       a += u32;
-      ustr += u8_mbtouc_unsafe (&u32, ustr, end - ustr);
+      ustr += u8_mbtouc (&u32, ustr, end - ustr);
       b += u32;
-      ustr += u8_mbtouc_unsafe (&u32, ustr, end - ustr);
+      ustr += u8_mbtouc (&u32, ustr, end - ustr);
       c += u32;
       mix (a, b, c);
       length -= 3;
     }
 
   /* Handle the last 3 elements's.  */
-  ustr += u8_mbtouc_unsafe (&u32, ustr, end - ustr);
-  a += u32;
-  if (--length)
+  if (length)
     {
-      ustr += u8_mbtouc_unsafe (&u32, ustr, end - ustr);
-      b += u32;
-      if (--length)
+      ustr += u8_mbtouc (&u32, ustr, end - ustr);
+      a += u32;
+      if (length > 1)
         {
-          ustr += u8_mbtouc_unsafe (&u32, ustr, end - ustr);
-          c += u32;
+          ustr += u8_mbtouc (&u32, ustr, end - ustr);
+          b += u32;
+          if (length > 2)
+            {
+              ustr += u8_mbtouc (&u32, ustr, end - ustr);
+              c += u32;
+            }
         }
+      final (a, b, c);
     }
 
-  final (a, b, c);
-
   if (sizeof (unsigned long) == 8)
     ret = (((unsigned long) c) << 32) | b;
   else
-- 
2.43.0

[PATCH v2 5/6] scm_i_utf8_string_hash: optimize ASCII

[Rob Browning]

Since we already compute the char length, use that to detect all ASCII
strings and handle those the same way we handle latin-1.

libguile/hash.c (scm_i_utf8_string_hash): when byte_len == char_len,
(i.e. fixed-width ASCII) optimize hashing via existing narrow path.
---
 libguile/hash.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/libguile/hash.c b/libguile/hash.c
index d92f60df8..bc65deb25 100644
--- a/libguile/hash.c
+++ b/libguile/hash.c
@@ -169,25 +169,29 @@ scm_i_latin1_string_hash (const char *str, size_t len)
 unsigned long 
 scm_i_utf8_string_hash (const char *str, size_t len)
 {
-  const uint8_t *end, *ustr = (const uint8_t *) str;
-  unsigned long ret;
-
-  /* The length of the string in characters.  This name corresponds to
-     Jenkins' original name.  */
-  size_t length;
-
-  uint32_t a, b, c, u32;
-
   if (len == (size_t) -1)
     len = strlen (str);
 
-  end = ustr + len;
-
+  const uint8_t *ustr = (const uint8_t *) str;
   if (u8_check (ustr, len) != NULL)
     /* Invalid UTF-8; punt.  */
     return scm_i_string_hash (scm_from_utf8_stringn (str, len));
 
-  length = u8_mbsnlen (ustr, len);
+  /* The length of the string in characters.  This name corresponds to
+     Jenkins' original name.  */
+  size_t length = u8_mbsnlen (ustr, len);
+
+  if (len == length) // ascii, same as narrow_string_hash above
+    {
+      unsigned long ret;
+      JENKINS_LOOKUP3_HASHWORD2 (str, len, ret);
+      ret >>= 2; /* Ensure that it fits in a fixnum.  */
+      return ret;
+    }
+
+  const uint8_t *end = ustr + len;
+  uint32_t a, b, c, u32;
+  unsigned long ret;
 
   /* Set up the internal state.  */
   a = b = c = 0xdeadbeef + ((uint32_t)(length<<2)) + 47;
-- 
2.43.0

[PATCH v2 6/6] define-meta-command: mention effects of a missing category

[Rob Browning]

module/system/repl/command.scm: add comment.
---
 module/system/repl/command.scm | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/module/system/repl/command.scm b/module/system/repl/command.scm
index ca7450610..8b0422dbd 100644
--- a/module/system/repl/command.scm
+++ b/module/system/repl/command.scm
@@ -235,6 +235,8 @@
      (define-meta-command ((name category) repl () . datums)
        docstring b0 b1 ...))
 
+    ;; These cases (with category #f) will only produce functional
+    ;; commands if the name is already in the *command-table*.
     ((_ (name repl (expression0 ...) . datums) docstring b0 b1 ...)
      (define-meta-command ((name #f) repl (expression0 ...) . datums)
        docstring b0 b1 ...))
-- 
2.43.0

Re: [PATCH v2 6/6] define-meta-command: mention effects of a missing category

[Rob Browning]

Rob Browning <rlb@defaultvalue.org> writes:

> module/system/repl/command.scm: add comment.

Pushed this one to main.

-- 
Rob Browning
rlb @defaultvalue.org and @debian.org
GPG as of 2011-07-10 E6A9 DA3C C9FD 1FF8 C676 D2C4 C0F0 39E9 ED1B 597A
GPG as of 2002-11-03 14DD 432F AE39 534D B592 F9A0 25C8 D377 8C7E 73A4