From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: ludo@gnu.org (Ludovic =?iso-8859-1?Q?Court=E8s?=) Newsgroups: gmane.lisp.guile.devel Subject: Re: review/merge request: wip-array-refactor Date: Thu, 23 Jul 2009 23:08:47 +0200 Message-ID: <87ab2vyun4.fsf@gnu.org> References: NNTP-Posting-Host: lo.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 8bit X-Trace: ger.gmane.org 1248383372 3853 80.91.229.12 (23 Jul 2009 21:09:32 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Thu, 23 Jul 2009 21:09:32 +0000 (UTC) To: guile-devel@gnu.org Original-X-From: guile-devel-bounces+guile-devel=m.gmane.org@gnu.org Thu Jul 23 23:09:25 2009 Return-path: Envelope-to: guile-devel@m.gmane.org Original-Received: from lists.gnu.org ([199.232.76.165]) by lo.gmane.org with esmtp (Exim 4.50) id 1MU5XW-0000bD-NI for guile-devel@m.gmane.org; Thu, 23 Jul 2009 23:09:23 +0200 Original-Received: from localhost ([127.0.0.1]:47427 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MU5XW-0003wU-3q for guile-devel@m.gmane.org; Thu, 23 Jul 2009 17:09:22 -0400 Original-Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MU5XL-0003sn-NU for guile-devel@gnu.org; Thu, 23 Jul 2009 17:09:11 -0400 Original-Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MU5XH-0003s0-6X for guile-devel@gnu.org; Thu, 23 Jul 2009 17:09:11 -0400 Original-Received: from [199.232.76.173] (port=35704 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MU5XH-0003rx-22 for guile-devel@gnu.org; Thu, 23 Jul 2009 17:09:07 -0400 Original-Received: from main.gmane.org ([80.91.229.2]:50251 helo=ciao.gmane.org) by monty-python.gnu.org with esmtps (TLS-1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1MU5XG-0001h3-4R for guile-devel@gnu.org; Thu, 23 Jul 2009 17:09:06 -0400 Original-Received: from list by ciao.gmane.org with local (Exim 4.43) id 1MU5XC-0004WF-Lx for guile-devel@gnu.org; Thu, 23 Jul 2009 21:09:02 +0000 Original-Received: from reverse-83.fdn.fr ([80.67.176.83]) by main.gmane.org with esmtp (Gmexim 0.1 (Debian)) id 1AlnuQ-0007hv-00 for ; Thu, 23 Jul 2009 21:09:02 +0000 Original-Received: from ludo by reverse-83.fdn.fr with local (Gmexim 0.1 (Debian)) id 1AlnuQ-0007hv-00 for ; Thu, 23 Jul 2009 21:09:02 +0000 X-Injected-Via-Gmane: http://gmane.org/ Original-Lines: 429 Original-X-Complaints-To: usenet@ger.gmane.org X-Gmane-NNTP-Posting-Host: reverse-83.fdn.fr X-URL: http://www.fdn.fr/~lcourtes/ X-Revolutionary-Date: 5 Thermidor an 217 de la =?iso-8859-1?Q?R=E9volution?= X-PGP-Key-ID: 0xEA52ECF4 X-PGP-Key: http://www.fdn.fr/~lcourtes/ludovic.asc X-PGP-Fingerprint: 821D 815D 902A 7EAB 5CEE D120 7FBA 3D4F EB1F 5364 X-OS: x86_64-unknown-linux-gnu User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.1.50 (gnu/linux) Cancel-Lock: sha1:SsSi6weAjEgHtHsyqCbUWfbgaxw= X-detected-operating-system: by monty-python.gnu.org: GNU/Linux 2.6, seldom 2.4 (older, 4) X-BeenThere: guile-devel@gnu.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "Developers list for Guile, the GNU extensibility library" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Original-Sender: guile-devel-bounces+guile-devel=m.gmane.org@gnu.org Errors-To: guile-devel-bounces+guile-devel=m.gmane.org@gnu.org Xref: news.gmane.org gmane.lisp.guile.devel:8929 Archived-At: Hello! Andy Wingo writes: > I've finished up my refactor of Guile's arrays. To my eye it's much > nicer now. Hey, great! Several general remarks: * Since I'm conservative and lazy, I'd have happily let this code rest in peace. ;-) In particular, last time I checked[*], we had poor coverage of {ramap,srfi-4,unif}.c so I'd be afraid of breaking something without noticing. [*] http://www.fdn.fr/~lcourtes/software/guile/coverage/libguile/index.html * As we discussed on IRC, I don't fully understand the rationale behind the SRFI-4/bytevector unification. For anything binary I/O, the bytevector API is probably better suited. A use case you once mentioned was graphics applications: they may get pixel streams (for which f32 vectors are a better fit than bytevectors) extracted after doing bytevector-based binary I/O. I'm not fully convinced that this use case justifies jointing the SRFI-4 and bytevector types. * Polymorphism is quite unusual in Scheme APIs. * There's definitely room for improvement, and factorization or the array/vector code, and a large part of what you've been doing here goes in the right direction. I agree with most of what Neil said, so I won't repeat it here. > commit 86d88a223c64276e7cd9b4503e7e2ecca5aae320 > Author: Andy Wingo > Date: Thu Jul 16 21:51:47 2009 +0200 > > remove deprecated functions from unif.c > > * libguile/unif.h: > * libguile/unif.c: Remove deprecated functions. > > * module/ice-9/deprecated.scm: Remove array-related deprecated > functions. > > * NEWS: Update. OK. (I assume the change in `NEWS' is to only keep "Changes in 1.9.2" at the top and remove "Changes in 1.9.1", right?) Can already be pushed to `master'. > commit 4b126598445c4f12c0aebca2adfaa45f3edd86ab > Author: Andy Wingo > Date: Thu Jul 16 22:02:25 2009 +0200 > > remove convert.{c,i.c,h} > > * libguile/convert.c: > * libguile/convert.h: > * libguile/convert.i.c: Remove these functions, which were undocumented, > not in the libguile/ header, and thus unlikely to have been used. OK; push to `master'. > commit a4a0d399c877cb802cdaf2c48713d3377a412c4f > Author: Andy Wingo > Date: Thu Jul 16 22:15:04 2009 +0200 > > clean up libguile/Makefile.am > > * libguile/Makefile.am: Clean up some of the file lists, should make > future diffs easier to parse. Good in principle. (In practice, I fear the day when I'll have to merge that one in the BDW-GC branch...) > commit 5d1b3b2db9349b615baac313ae5a111fa68573ac > Author: Andy Wingo > Date: Fri Jul 17 00:25:49 2009 +0200 > > rename ramap.[ch] to array-map.[ch] Neil is concerned about the header renaming. I think it's OK because individual headers are not meant to be included individually, i.e., one is supposed to always #include (except for ). > commit 2a610be59412a9d633a373c6f6ec4d4794c40fd8 > Author: Andy Wingo > Date: Sun Jul 19 15:04:40 2009 +0200 > > add generic array implementation facility Excellent! I would just add comments to `scm_t_array_implementation', and pipe the changes through GNU Ident. :-) Also: +typedef enum { + SCM_ARRAY_ELEMENT_TYPE_SCM = 0, /* SCM values */ + SCM_ARRAY_ELEMENT_TYPE_CHAR = 1, /* characters */ [...] + SCM_ARRAY_ELEMENT_TYPE_C64 = 15, + SCM_ARRAY_ELEMENT_TYPE_LAST = 15, +} scm_t_array_element_type; Per C99 (and previous versions) §6.7.2.2, this is not necessary: If the first enumerator has no =, the value of its enumeration constant is 0. Each subsequent enumerator with no = defines its enumeration constant as the value of the constant expression obtained by adding 1 to the value of the previous enumeration constant. Up-to-here, I think the changes are mostly non-controversial and would be OK for 1.9 IMO. > commit 66b9d7d304a349d5bb4f763a47143f09da58d97f > Author: Andy Wingo > Date: Fri Jul 17 12:45:24 2009 +0200 > > remove enclosed arrays Same as Neil: I'd like to avoid gratuitous incompatibilities if it can be avoided. > commit f45eccffa73c043466a4cc0f5037132ee5795eee > Author: Andy Wingo > Date: Sat Jul 18 12:43:54 2009 +0200 > > add registry of vector constructors, make-generalized-vector > > * libguile/generalized-vectors.h: > * libguile/generalized-vectors.c: Add a registry of vector constructors. > (scm_make_generalized_vector): New public function, constructs a > vector of a given type. > > * libguile/bitvectors.c: > * libguile/bytevectors.c: > * libguile/srfi-4.c: > * libguile/strings.c: > * libguile/vectors.c: Register vector constructors. > > * libguile/extensions.c (scm_init_extensions): No need to NULL the list > of registered extensions here, the static init does it for us. Allows > scm_c_register_extension to be called before scm_init_extensions. This `extensions.c' change should be in a separate commit, I think. > * libguile/init.c (scm_i_init_guile): Move array initialization earlier, > so e.g. scm_init_strings has access to a valid list of array element > types when registering its vector constructor. This: -static extension_t *registered_extensions; +static extension_t *registered_extensions = NULL; is not needed. Otherwise OK. > commit 943a0a8759504c4a367c1904bef4a8afbc6208dd > Author: Andy Wingo > Date: Sat Jul 18 12:58:37 2009 +0200 > > make-typed-array builds backing vector via make-generalized-vector > > * libguile/arrays.c: Rework to use scm_make_generalized_vector instead > of our own type table. > > * libguile/bitvectors.c: Fix some includes. OK. (Is the latter related to the former?) > commit ac8ed3db31769d7ede5e75fba1697e8dde55fae4 > Author: Andy Wingo > Date: Sat Jul 18 13:26:18 2009 +0200 > > any->u8vector and family now implemented in Scheme > > * module/Makefile.am: > * module/srfi/srfi-4/gnu.scm: New module, for extensions to srfi-4. > Currently defines the any->FOOvector family. > > * libguile/srfi-4.c: > * libguile/srfi-4.i.c: Dispatch scm_any_to_FOOvector calls to the > scheme-implemented functions in (srfi srfi-4 gnu). +#define DEFINE_SCHEME_PROXY100(cname, modname, scmname) \ I'd remove "100"... +#define DEFPROXY100(cname, scmname) \ + DEFINE_SCHEME_PROXY100 (cname, MOD, scmname) + +#define DEFINE_SRFI_4_GNU_PROXIES(tag) \ + DEFPROXY100 (scm_any_to_##tag##vector, "any->" #tag "vector") ... and keep only one macro, as Neil suggested. +DEFINE_SRFI_4_GNU_PROXIES (u8); Remove extraneous semicolons. +;;; Extensions to SRFI-4 + +;; Copyright (C) 2001, 2002, 2004, 2006, 2009 Free Software Foundation, Inc. Should be just "2009". > commit f332089ed43761440a2a8c272ee61a709b38cc24 > Author: Andy Wingo > Date: Sat Jul 18 13:46:29 2009 +0200 > > bytevector inlinedness indicated by flag, not length > > * libguile/bytevectors.h (SCM_BYTEVECTOR_INLINE_P): Change to check a > flag instead of checking the length of the bytevector. > > * libguile/bytevectors.c (make_bytevector_from_buffer): Handle the len > <= inline threshold case as well. Set the inline flag as appropriate. > (make_bytevector): Updat the inline flag as appropriate. > (scm_c_take_bytevector): Just dispatch to make_bytevector_from_buffer. > (scm_i_shrink_bytevector): Update the inline flag as appropriate. > Update the length when shrinking an already-inlined vector. > (STRING_TO_UTF): Fix some indentation. The scm_c_take_bytevector -> make_bytevector_from_buffer shuffling can be avoided. static inline SCM make_bytevector (size_t len) { - SCM bv; - if (SCM_UNLIKELY (len == 0)) - bv = scm_null_bytevector; + return scm_null_bytevector; + + if (SCM_BYTEVECTOR_INLINEABLE_SIZE_P (len)) + { + SCM ret; + SCM_NEWSMOB2 (ret, scm_tc16_bytevector, len, NULL); + SCM_BYTEVECTOR_SET_INLINE (ret); + return ret; + } The code initially had a single `return' statement per function, to improve readability (IMO). Do you think otherwise? > commit e286c973fcd63c0930d9302cc5f1a280b9b22615 > Author: Andy Wingo > Date: Sun Jul 19 15:11:53 2009 +0200 > > bytevectors have "element type" field, e.g. for generalized-vector-ref > > Bytevectors have a very close relationship to other forms of uniform > vectors. Often you want to view a u64vector as a series of bytes, for > writing over a socket; or to process an incoming stream using the > convenient and less error-prone s16vector-ref API rather than > bytevector-s16-native-ref. > > The essential needs of the representation of a bytevector and an > s64vector are the same, so we take advantage of that and extend the > bytevector implementation to have a "native type" field, which defaults > to VU8. This comments belong to a header (info "(standards) Change Log Concepts"). > This commit doesn't actually expose any user-noticeable changes, > however. > > * libguile/bytevectors.h (SCM_BYTEVECTOR_ELEMENT_TYPE): New internal > defines. > (scm_i_make_typed_bytevector, scm_c_take_typed_bytevector): New > internal functions. > > * libguile/bytevectors.c (SCM_BYTEVECTOR_SET_ELEMENT_TYPE): > (SCM_BYTEVECTOR_TYPE_SIZE): > (SCM_BYTEVECTOR_TYPED_LENGTH): New internal macros. > (make_bytevector, make_bytevector_from_buffer): Take an extra > argument, the element type. The length argument is interpreted as > being the number of elements, which corresponds to the number of bytes > in the default VU8 case. Doing it this way eliminates a class of bugs > -- e.g. a u32vector of length 3 bytes doesn't make sense. We do have > to check for another class of bugs: overflow. The length stored on the > bytevector itself is still the byte length, though. > (scm_i_make_typed_bytevector): > (scm_c_take_typed_bytevector): New internal functions. > (scm_i_shrink_bytevector): Make sure the new size is valid for the > bytevector's type. > (scm_i_bytevector_generalized_set_x): Remove this function, the > array-handle infrastructure takes care of this for us. > (print_bytevector): Print the bytevector according to its type. > (scm_make_bytevector, scm_bytevector_copy) > (scm_uniform_array_to_bytevector) > (scm_u8_list_to_bytevector, scm_bytevector_to_uint_list): Adapt to > make_bytevector extra arg. > (bv_handle_ref, bv_handle_set_x): Adapt to ref and set based on the > type of the bytevector, e.g. f64 or u8. > (bytevector_get_handle): Set the typed length of the vector, not the > byte length. > > Conflicts: > > libguile/bytevectors.c At this point, bytevectors are aware of all the other vector types. In principle, I don't like this idea. I don't have a clear picture of the array/vector dependency graph, though. This: + if (SCM_UNLIKELY (len == 0 && element_type == 0)) should be "element_type == SCM_ARRAY_ELEMENT_TYPE_SCM". > commit cd43fdc5b7a7c851ee0f2b4e96a1f394fb50d869 > Author: Andy Wingo > Date: Sat Jul 18 19:03:28 2009 +0200 > > fix (bytevector-ieee-single-native-set! x 0 0) > > * libguile/bytevectors.c (VALIDATE_REAL): SCM_VALIDATE_REAL is not what > we need for checking values for bytevector-ieee-single-native-set! et > al, so define our own validator. > (IEEE754_SET, IEEE754_NATIVE_SET): Use it. Can you please push it to `master', preferably with a test case? > commit 6f4895577734bb107d488f31dca82d5ad4c25a65 > Author: Andy Wingo > Date: Sun Jul 19 15:35:33 2009 +0200 > > reimplement srfi-4 vectors on top of bytevectors > > * libguile/srfi-4.h: > * libguile/srfi-4.c (scm_make_srfi_4_vector): New function, exported by > (srfi srfi-4 gnu). > * libguile/srfi-4.i.c: Removed. > * module/srfi/srfi-4.scm: > * module/srfi/srfi-4/gnu.scm: Reimplement srfi-4 vectors on top of > bytevectors. The implementation is mostly in Scheme now. I appreciate the move to Scheme. ;-) > * module/rnrs/bytevector.scm: > * libguile/bytevectors.c (scm_uniform_array_to_bytevector): No more need > for this, as uniform vectors are bytevectors, and we can get the > backing vector of an array via shared-array-root. > > * libguile/bytevectors.c (bytevector_ref_c32, bytevector_ref_c64) > (bytevector_set_c32, bytevector_set_c64): Fix some embarrassing bugs. > Still need to do an upper bounds check. Hmm... > * libguile/deprecated.h: Remove deprecated array functions: > scm_i_arrayp, scm_i_array_ndim, scm_i_array_mem, scm_i_array_v, > scm_i_array_base, scm_i_array_dims, and the deprecated macros: > SCM_ARRAYP, SCM_ARRAY_NDIM, SCM_ARRAY_CONTP, SCM_ARRAY_MEM, > SCM_ARRAY_V, SCM_ARRAY_BASE, SCM_ARRAY_DIMS. > * libguile/deprecated.c (scm_uniform_vector_read_x) > (scm_uniform_vector_write, scm_uniform_array_read_x) > (scm_uniform_array_write): Newly deprecated functions. > > * libguile/generalized-arrays.c (scm_array_type): Remove the bytevector > hack. This does introduce the bug that #vu8(1 2 3) will compile to > #u8(1 2 3). I'm working on that. > > * libguile/objcodes.c (scm_bytecode_to_objcode, scm_objcode_to_bytecode): > Rework to operate on bytevectors, as scm_make_u8vector now causes a > module lookup, which can't be done e.g. when loading the VM boot > program for psyntax-pp.go on a fresh bootstrap. > > * libguile/objcodes.h (SCM_F_OBJCODE_IS_BYTEVECTOR): > (SCM_OBJCODE_IS_BYTEVECTOR): s/U8VECTOR/BYTEVECTOR/. > > * module/ice-9/boot-9.scm (the-scm-module): A terrible hack to pull in > (srfi srfi-4), as the bindings are primarily there now. We'll worry > about this later. The issue is that `make-c32vector' et al., which used to be in `the-scm-module' are now confined in `(srfi srfi-4 gnu)'. There's a compatibility issue here. Perhaps it should be pulled in as well? > * module/language/glil/compile-assembly.scm (dump-object): Update to > work with array-contents and shared-array-root. > > * test-suite/tests/bytevectors.test: Remove uniform-array->bytevector > test. > commit ae3543b1f27e973b25060f71f095bf23ef149aee > Author: Andy Wingo > Date: Sat Jul 18 19:08:43 2009 +0200 > > clean up includes in vectors.[ch] > > * libguile/vectors.h: > * libguile/vectors.c: Clean up the includes... mostly. "Mostly"? What does it clean up? +#include "libguile/arrays.h" /* Hit me with the ugly stick */ Does that mean there are circular dependencies or something like that? Waouh, it's a lot of work, and reviewing it takes some time, too. Honestly, I'd rather spend the small amount of time I spend on Guile these days in other areas with higher priorities. Thanks, Ludo'.