From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Andy Wingo Newsgroups: gmane.lisp.guile.devel Subject: Re: [PATCH] Add private port structure, and move iconv descriptors there Date: Wed, 27 Mar 2013 21:51:06 +0100 Message-ID: <87a9poin2d.fsf@pobox.com> References: <87vc9ij5z0.fsf@pobox.com> <87fw0l2yyk.fsf@gnu.org> <877gltxgrg.fsf_-_@tines.lan> <87y5e8stst.fsf@pobox.com> <87d2vdekws.fsf@tines.lan> <87d2uk62a0.fsf_-_@tines.lan> NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain X-Trace: ger.gmane.org 1364417490 7416 80.91.229.3 (27 Mar 2013 20:51:30 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Wed, 27 Mar 2013 20:51:30 +0000 (UTC) Cc: Ludovic =?utf-8?Q?Court=C3=A8s?= , guile-devel@gnu.org To: Mark H Weaver Original-X-From: guile-devel-bounces+guile-devel=m.gmane.org@gnu.org Wed Mar 27 21:51:56 2013 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 1UKxJy-00083I-HF for guile-devel@m.gmane.org; Wed, 27 Mar 2013 21:51:46 +0100 Original-Received: from localhost ([::1]:40784 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UKxJa-0006wO-JB for guile-devel@m.gmane.org; Wed, 27 Mar 2013 16:51:22 -0400 Original-Received: from eggs.gnu.org ([208.118.235.92]:35208) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UKxJV-0006w5-N1 for guile-devel@gnu.org; Wed, 27 Mar 2013 16:51:19 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UKxJT-0001hg-NE for guile-devel@gnu.org; Wed, 27 Mar 2013 16:51:17 -0400 Original-Received: from a-pb-sasl-quonix.pobox.com ([208.72.237.25]:61633 helo=sasl.smtp.pobox.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UKxJT-0001gm-I5; Wed, 27 Mar 2013 16:51:15 -0400 Original-Received: from sasl.smtp.pobox.com (unknown [127.0.0.1]) by a-pb-sasl-quonix.pobox.com (Postfix) with ESMTP id 37175CFC1; Wed, 27 Mar 2013 16:51:10 -0400 (EDT) DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=pobox.com; h=from:to:cc :subject:references:date:in-reply-to:message-id:mime-version :content-type; s=sasl; bh=JnlRXEree2N5dMtJFa+6NCt5cPI=; b=FDcJkc kcHgh3KsMoe4FHKgUQsZnqVZ3w5Pj3OyxyAnGrvmgEDBKbQNVFBYEblfSdCUxCvm eRXmT//4trL4ubZ1o6IP8e2lgvhQnqYy34YLim8kPLHiKdur6B7HkIx23FuPZbfs 20xDi+I2Gpz1QBgM4p8ffC8g/MF/I9VI5g5Cc= DomainKey-Signature: a=rsa-sha1; c=nofws; d=pobox.com; h=from:to:cc :subject:references:date:in-reply-to:message-id:mime-version :content-type; q=dns; s=sasl; b=pPS/aytZIIKDyrehQErObt0KpYsvOl5O EUVCWVRJbxsL9R8TQPH/c+sH0yBuw4hBUNY/daYWwuRxbJJx108kRVNEEoLUqLyO oa859a55VyG8E20abV0FMAVjVPTl98vWYkdFeogWjFB8TaQgUz5RtsWQvci7ZUCA X3z3zNUYA2k= Original-Received: from a-pb-sasl-quonix.pobox.com (unknown [127.0.0.1]) by a-pb-sasl-quonix.pobox.com (Postfix) with ESMTP id 2F298CFBF; Wed, 27 Mar 2013 16:51:10 -0400 (EDT) Original-Received: from badger (unknown [88.160.190.192]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by a-pb-sasl-quonix.pobox.com (Postfix) with ESMTPSA id 7F48DCFBA; Wed, 27 Mar 2013 16:51:09 -0400 (EDT) In-Reply-To: <87d2uk62a0.fsf_-_@tines.lan> (Mark H. Weaver's message of "Wed, 27 Mar 2013 16:00:55 -0400") User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.2 (gnu/linux) X-Pobox-Relay-ID: 0D8A4280-9720-11E2-954C-D36F0E5B5709-02397024!a-pb-sasl-quonix.pobox.com X-detected-operating-system: by eggs.gnu.org: Solaris 10 X-Received-From: 208.72.237.25 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:16032 Archived-At: Hi, On Wed 27 Mar 2013 21:00, Mark H Weaver writes: > index 8737a76..45e770d 100644 > --- a/libguile/ports.c > +++ b/libguile/ports.c > @@ -60,6 +60,8 @@ > #include "libguile/fluids.h" > #include "libguile/eq.h" > > +#include "libguile/private-ports.h" > + > #ifdef HAVE_STRING_H > #include > #endif > @@ -589,10 +591,10 @@ finalize_port (void *ptr, void *data) > > entry = SCM_PTAB_ENTRY (port); > > - if (entry->input_cd != (iconv_t) -1) > - iconv_close (entry->input_cd); > - if (entry->output_cd != (iconv_t) -1) > - iconv_close (entry->output_cd); > + if (entry->internal->input_cd != (iconv_t) -1) > + iconv_close (entry->internal->input_cd); > + if (entry->internal->output_cd != (iconv_t) -1) > + iconv_close (entry->internal->output_cd); > > SCM_SETSTREAM (port, 0); > SCM_CLR_PORT_OPEN_FLAG (port); I would appreciate a symmetry between the name of the internal member and the name of the header. WDYT about "internal" in both? I also think that "ports-internal.h" is a better name than "internal-ports.h". > + entry->internal = (struct scm_t_port_private *) > + scm_gc_calloc (sizeof (struct scm_t_port_private), "port-private"); Would be nice to have a macro to allocate structs, with the correct type. Like #define scm_gc_typed_calloc(t) (((t)*) scm_gc_calloc (sizeof (t), #t)) > @@ -1297,12 +1307,14 @@ get_iconv_codepoint (SCM port, scm_t_wchar *codepoint, > char buf[SCM_MBCHAR_BUF_SIZE], size_t *len) > { > scm_t_port *pt; > + struct scm_t_port_private *pti; Indeed here the name belies your intention ;) Seems you want to call the type scm_t_port_internal. (Though, do we need _t_ in struct tags? Seems redundant, given that struct tags are a separate namespace. Dunno) > @@ -1332,7 +1344,7 @@ get_iconv_codepoint (SCM port, scm_t_wchar *codepoint, > input_left = bytes_consumed + 1; > output_left = sizeof (utf8_buf); > > - done = iconv (pt->input_cd, &input, &input_left, > + done = iconv (pti->input_cd, &input, &input_left, > &output, &output_left); Would be nice to fix up tabs here > @@ -1369,15 +1381,22 @@ get_codepoint (SCM port, scm_t_wchar *codepoint, > int err; > scm_t_port *pt = SCM_PTAB_ENTRY (port); > > - if (pt->input_cd == (iconv_t) -1) > + if (pt->internal->encoding_type == SCM_PORT_ENCODING_TYPE_UNINITIALIZED) Here I would appreciate consonance with the SCM_PORT_ENCODING_MODE in `master'. > + struct scm_t_port_private *pti; > iconv_t new_input_cd, new_output_cd; > + enum scm_t_port_encoding_type new_encoding_type; In general we use typedefs for enums in guile, fwiw So currently this patch is not OK for me, because of the interaction with master. Can you take a look at porting 6c98257f2ead0855f218369ea7f9a823cdb9727e? This work you are doing will not be easy to merge forward. If you are willing to take care of that, I think I will be OK with something going into stable-2.0 after this review is over. If not, I would prefer some other solution (for example one of the 3 or 4 BOM-related patches I made that you rejected ;-) Thanks, Andy -- http://wingolog.org/