From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Mark H Weaver Newsgroups: gmane.lisp.guile.devel Subject: Re: [PATCH] Per-port read options, reader directives, SRFI-105 Date: Wed, 24 Oct 2012 10:41:47 -0400 Message-ID: <874nlkx784.fsf@tines.lan> References: <87a9vmvh9s.fsf@tines.lan> <87mwzdzpqf.fsf@tines.lan> <87pq4898xh.fsf@gnu.org> <87bofsy0qw.fsf@tines.lan> <87k3ug6mjj.fsf@gnu.org> NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Trace: ger.gmane.org 1351089745 3916 80.91.229.3 (24 Oct 2012 14:42:25 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Wed, 24 Oct 2012 14:42:25 +0000 (UTC) Cc: guile-devel@gnu.org To: ludo@gnu.org (Ludovic =?utf-8?Q?Court=C3=A8s?=) Original-X-From: guile-devel-bounces+guile-devel=m.gmane.org@gnu.org Wed Oct 24 16:42:33 2012 Return-path: Envelope-to: guile-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1TR2AD-0002fi-8p for guile-devel@m.gmane.org; Wed, 24 Oct 2012 16:42:33 +0200 Original-Received: from localhost ([::1]:37829 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TR2A5-0007mO-Jr for guile-devel@m.gmane.org; Wed, 24 Oct 2012 10:42:25 -0400 Original-Received: from eggs.gnu.org ([208.118.235.92]:32813) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TR29t-0007jW-Ox for guile-devel@gnu.org; Wed, 24 Oct 2012 10:42:23 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TR29j-0008W5-Pg for guile-devel@gnu.org; Wed, 24 Oct 2012 10:42:13 -0400 Original-Received: from world.peace.net ([96.39.62.75]:39991) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TR29j-0008W1-K8; Wed, 24 Oct 2012 10:42:03 -0400 Original-Received: from 209-6-91-212.c3-0.smr-ubr1.sbo-smr.ma.cable.rcn.com ([209.6.91.212] helo=tines.lan) by world.peace.net with esmtpsa (TLS1.0:DHE_RSA_AES_128_CBC_SHA1:16) (Exim 4.72) (envelope-from ) id 1TR29a-0004sa-Q7; Wed, 24 Oct 2012 10:41:55 -0400 In-Reply-To: <87k3ug6mjj.fsf@gnu.org> ("Ludovic \=\?utf-8\?Q\?Court\=C3\=A8s\=22'\?\= \=\?utf-8\?Q\?s\?\= message of "Wed, 24 Oct 2012 15:13:04 +0200") User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.2 (gnu/linux) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6 (newer, 3) X-Received-From: 96.39.62.75 X-BeenThere: guile-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Developers list for Guile, the GNU extensibility library" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guile-devel-bounces+guile-devel=m.gmane.org@gnu.org Original-Sender: guile-devel-bounces+guile-devel=m.gmane.org@gnu.org Xref: news.gmane.org gmane.lisp.guile.devel:15044 Archived-At: ludo@gnu.org (Ludovic Court=C3=A8s) writes: > Mark H Weaver skribis: >> ludo@gnu.org (Ludovic Court=C3=A8s) writes: >>> So, what about exposing a =E2=80=98set-port-read-options!=E2=80=99 proc= edure, and then >>> using it to write tests? >> >> That's a lot of extra work. It means designing, implementing, and >> documenting a new non-trivial API that we'll have to maintain forever. >> I'd rather not do that work now. I'm quite overloaded and have more >> important things to do. >> >> Can the API be added later, by someone who is motivated to do that work? > > Yeah, we can think about it later. The thing is, that API exists in > read.c anyway, so I didn=E2=80=99t think it would be so much extra work. APIs that we expose to the outside world need to be maintained approximately forever, so we should expend a great deal of effort to make sure they are future proof. We don't have to worry so much about a private interface that's accessible only within read.c. > Now, I agree that the less we expose, the better. ;-) At least until we have the time to come up with a good interface. >>> Mark H Weaver skribis: >>>> +set_per_port_read_option (SCM port, int shift, int value) >>> >>> Also change =E2=80=98shift=E2=80=99 to =E2=80=98option=E2=80=99, and = =E2=80=98int value=E2=80=99 to something like >>> =E2=80=98enum t_option_state value=E2=80=99, where: >>> >>> enum t_option_state >>> { >>> OPTION_INHERITED, /* global option setting inherited */ >>> OPTION_DISABLED, >>> OPTION_ENABLED >>> }; >>> >>> the goal being to hide as much of the bit-twiddling as possible. >> >> Right now, this single function can be used for all the options (both >> the boolean options and the keyword style option). If I change it as >> you suggest, then I would have to split it into two nearly-identical >> functions, and it wouldn't hide _any_ bit-twiddling. Apart from >> duplicating the code, the only changes would be to rename >> OVERRIDE_DEFAULT to OPTION_INHERITED, and to make the non-inherit case >> more complex by changing a simple assignment (of the 2-bit bit-field >> into scm_t_read_opts) into a switch statement to convert these new enum >> values into a value appropriate for scm_t_read_opts. >> >> Is this added complexity really necessary? This is all internal logic >> that's confined to a few static functions in read.c. > > Well, I was more thinking in terms of the interface I=E2=80=99d like for = the > concepts at hand: we have per-ports and global settings, which we want > to manipulate, and we want to know which ones are applicable at a given > point. > > Thus, I thought we=E2=80=99d logically have these 3 functions: > set_port_read_options, port_read_options, and applicable_read_options. Logically, I agree that this would be a nice interface. The problem is really one of efficiency. It's quite expensive to access the per-port read options directly, because it requires locking the port table mutex, doing a hash table lookup, and then an alist lookup. That's not something I want to do more than once per call to 'read'. (Even doing it once is slightly painful). Efficiency is the main reason that I chose to compute all of the applicable read options and place them in OPTS at the start of 'read'. Efficiency is also the reason that I packed all of the read option overrides into a single integer. > Whether these are implemented in terms of bit fields is not the first > thing I want to see when I open read.c. > > Perhaps this is just a matter of presentation, but my impression was > that set_port_read_options and the various constants would force me to > think in terms of bit-twiddling more than in terms or read options. FWIW, all of the details of the bit-twiddling and the storage mechanism of per-port read options are confined to just two static functions: 'init_read_options' and 'set_per_port_read_option'. The rest of read.c needn't think about bit-twiddling at all. The relevant interface for the rest of read.c is as follows: * Look up applicable read options in OPTS. * Set per-port read options by calling 'set_per_port_*'. So nothing else need think about the bit-twiddling. That said, I agree that it's unfortunate to see this bit-twiddling at the beginning of read.c. How about moving it to the end? :) What do you think? Mark