From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 32058 invoked by alias); 15 Mar 2008 01:02:37 -0000 Received: (qmail 32024 invoked by uid 22791); 15 Mar 2008 01:02:31 -0000 X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (66.187.233.31) by sourceware.org (qpsmtpd/0.31) with ESMTP; Sat, 15 Mar 2008 01:02:06 +0000 Received: from int-mx1.corp.redhat.com (int-mx1.corp.redhat.com [172.16.52.254]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id m2F11qqt010140; Fri, 14 Mar 2008 21:02:04 -0400 Received: from pobox.corp.redhat.com (pobox.corp.redhat.com [10.11.255.20]) by int-mx1.corp.redhat.com (8.13.1/8.13.1) with ESMTP id m2F11qGP016679; Fri, 14 Mar 2008 21:01:52 -0400 Received: from opsy.redhat.com (vpn-248-6.boston.redhat.com [10.13.248.6]) by pobox.corp.redhat.com (8.13.1/8.13.1) with ESMTP id m2F11pCr013871; Fri, 14 Mar 2008 21:01:52 -0400 Received: by opsy.redhat.com (Postfix, from userid 500) id BB4033782EC; Fri, 14 Mar 2008 18:09:24 -0600 (MDT) To: Kris Van Hees Cc: gcc-patches@gcc.gnu.org Subject: Re: [PATCH] utf-16 and utf-32 support in C and C++ References: <20080313193208.GE19427@oracle.com> From: Tom Tromey Reply-To: tromey@redhat.com X-Attribution: Tom Date: Sat, 15 Mar 2008 02:57:00 -0000 In-Reply-To: <20080313193208.GE19427@oracle.com> (Kris Van Hees's message of "Thu\, 13 Mar 2008 15\:32\:08 -0400") Message-ID: User-Agent: Gnus/5.11 (Gnus v5.11) Emacs/22.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org X-SW-Source: 2008-03/txt/msg00926.txt.bz2 >>>>> "Kris" == Kris Van Hees writes: Kris> * include/cpp-id-data.h (UC): Was U, conflicts with U"..." Kris> literal. This renaming is fine and if you want to commit it as a separate patch, that would be ok. But, if you'd rather just keep it all as one big patch, that is also ok by me. I haven't read the spec. What does it say about surrogate characters? The code seems to just defer to whatever iconv does. I also have a few stylistic nits, nothing serious. Kris> + switch (type) { Brace on a new line. There are a couple instances of this with 'switch'. Kris> tree Kris> fix_string_type (tree value) [...] Kris> + if (wide) { Kris> + if (TREE_TYPE (value) == char16_array_type_node) { Braces on new lines, lots of instances in this function. Kris> + type = *base == 'L' ? CPP_WSTRING Kris> + : *base == 'U' ? CPP_STRING32 Kris> + : *base == 'u' ? CPP_STRING16 Kris> + : CPP_STRING; Multi-line expressions like need parens around the RHS of the assignment. And they have to be indented a bit differently. I think the coding standards explain the details. Kris> + if (width != CPP_OPTION(pfile, char_precision)) Space before the paren. Kris> +static struct cset_converter Kris> +convertor_for_type (cpp_reader *pfile, enum cpp_ttype type) Please use "converter", not "convertor". The former is already in use in the source, and also is hugely more popular according to google. Kris> + cpp_string *to, enum cpp_ttype type) Extra space before 'enum'. The nit-pickiest! Tom