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 00:04:07 -0400 Message-ID: <87bofsy0qw.fsf@tines.lan> References: <87a9vmvh9s.fsf@tines.lan> <87mwzdzpqf.fsf@tines.lan> <87pq4898xh.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 1351051479 9165 80.91.229.3 (24 Oct 2012 04:04:39 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Wed, 24 Oct 2012 04:04:39 +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 06:04:47 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 1TQsD0-000725-3v for guile-devel@m.gmane.org; Wed, 24 Oct 2012 06:04:46 +0200 Original-Received: from localhost ([::1]:45172 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TQsCs-0005gV-4I for guile-devel@m.gmane.org; Wed, 24 Oct 2012 00:04:38 -0400 Original-Received: from eggs.gnu.org ([208.118.235.92]:35011) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TQsCp-0005gC-1F for guile-devel@gnu.org; Wed, 24 Oct 2012 00:04:36 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TQsCn-0004xr-FS for guile-devel@gnu.org; Wed, 24 Oct 2012 00:04:34 -0400 Original-Received: from world.peace.net ([96.39.62.75]:39377) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TQsCn-0004wv-A4; Wed, 24 Oct 2012 00:04:33 -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 1TQsCU-0003h3-I3; Wed, 24 Oct 2012 00:04:14 -0400 In-Reply-To: <87pq4898xh.fsf@gnu.org> ("Ludovic \=\?utf-8\?Q\?Court\=C3\=A8s\=22'\?\= \=\?utf-8\?Q\?s\?\= message of "Tue, 23 Oct 2012 23:26:34 +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:15039 Archived-At: Hi Ludovic, Thanks for your review. I have pushed the first six patches, with all of your suggestions applied except that I didn't make 'scm_t_read_opts' const, because it needs to be mutated when encountering a reader directive, as you later acknowledged on IRC. ludo@gnu.org (Ludovic Court=C3=A8s) writes: > Mark H Weaver skribis: >> From 255aaaf0f474d45bd67d6b3b102b2806a8f0db97 Mon Sep 17 00:00:00 2001 >> From: Mark H Weaver >> Date: Tue, 23 Oct 2012 00:50:42 -0400 >> Subject: [PATCH 7/9] Implement per-port read options. >> >> * libguile/read.c (scm_t_read_opts): Update comment to mention the >> per-port read options. >> >> (sym_read_option_overrides): New symbol. >> >> (set_per_port_read_option): New internal static function. > > The patch add this static function, but leaves it unused. And also, > there are no tests. That's because you asked me to split the patch into pieces :-P It only causes a warning, and the immediately following patch makes use of this function and tests the functionality. > So, what about exposing a =E2=80=98set-port-read-options!=E2=80=99 proced= ure, 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? [... skipped several more comments that I agree with ...] >> +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. > What about also adding: > > static int per_port_read_option (SCM port, int option); > static int applicable_read_option (SCM port, int option); Who would use these functions? >> +/* Initialize the internal read options structure from the global and >> + per-port read options. */ >> +static void >> +init_read_options (SCM port, scm_t_read_opts *opts) > > Rather along the lines of =E2=80=9CInitialize OPTS based on PORT=E2=80=99= s read options > and the global read options.=E2=80=9D Okay. >> +#define RESOLVE_BOOLEAN_OPTION(NAME, name) \ >> + do { \ >> + x =3D OVERRIDE_MASK & (overrides >> OVERRIDE_SHIFT_ ## NAME); \ >> + if (x =3D=3D OVERRIDE_DEFAULT) \ >> + x =3D !!SCM_ ## NAME; \ >> + opts->name =3D x; \ >> + } while (0) > > Braces misplaced. Okay. Thanks, Mark