From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 9719 invoked by alias); 14 Sep 2011 01:53:02 -0000 Received: (qmail 9694 invoked by uid 22791); 14 Sep 2011 01:52:58 -0000 X-SWARE-Spam-Status: No, hits=-5.2 required=5.0 tests=AWL,BAYES_50,RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD,SPF_HELO_PASS,TW_CD,TW_CP,TW_CX X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 14 Sep 2011 01:52:29 +0000 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p8E1qTk7030317 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Tue, 13 Sep 2011 21:52:29 -0400 Received: from ns3.rdu.redhat.com (ns3.rdu.redhat.com [10.11.255.199]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id p8E1qS75027101; Tue, 13 Sep 2011 21:52:28 -0400 Received: from [0.0.0.0] (ovpn-113-157.phx2.redhat.com [10.3.113.157]) by ns3.rdu.redhat.com (8.13.8/8.13.8) with ESMTP id p8E1qRSc007743; Tue, 13 Sep 2011 21:52:27 -0400 Message-ID: <4E7008DA.6090703@redhat.com> Date: Wed, 14 Sep 2011 08:00:00 -0000 From: Jason Merrill User-Agent: Mozilla/5.0 (X11; Linux i686; rv:6.0.2) Gecko/20110906 Thunderbird/6.0.2 MIME-Version: 1.0 To: Ed Smith-Rowland <3dw4rd@verizon.net> CC: gcc-patches Subject: Re: [C++-11] User defined literals References: <4E6F6A1C.90305@verizon.net> In-Reply-To: <4E6F6A1C.90305@verizon.net> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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: 2011-09/txt/msg00830.txt.bz2 Since we're starting to come up on the end of stage 1, I'll go ahead and review the current patch even though it isn't finished yet. Thanks for all your work on this, it definitely seems to be coming together. On 09/13/2011 10:35 AM, Ed Smith-Rowland wrote: > +#define CPP_N_USERDEF 0x1000000 /* C++0x user-defned literal. */ "defined" > -extern unsigned cpp_classify_number (cpp_reader *, const cpp_token *); > +extern unsigned cpp_classify_number (cpp_reader *, const cpp_token *, > + char **ud_suffix); > + > +/* Return the classification flags for a float suffix. */ > +unsigned int cpp_interpret_float_suffix (const char *s, size_t len); > + > +/* Return the classification flags for an int suffix. */ > +unsigned int cpp_interpret_int_suffix (const char *s, size_t len); > > /* Evaluate a token classified as category CPP_N_INTEGER. */ > extern cpp_num cpp_interpret_integer (cpp_reader *, const cpp_token *, Let's follow the pattern of the existing declarations by adding explicit 'extern' and not naming the parameters. > @@ -327,6 +327,10 @@ pp_cxx_constant (cxx_pretty_printer *pp, tree t) > + case USERDEF_LITERAL: > + pp_c_constant (pp_c_base (pp), USERDEF_LITERAL_VALUE (t)); > + break; > @@ -1755,6 +1757,11 @@ dump_expr (tree t, int flags) > + case USERDEF_LITERAL: > + pp_constant (cxx_pp, USERDEF_LITERAL_VALUE (t)); > + dump_decl (USERDEF_LITERAL_SUFFIX_ID (t), flags); > + break; These should be the same--or just call pp_cxx_constant from dump_expr. > +/* Extract the suffix from a user-defined literal string or char. */ > +void > +cpp_get_userdef_suffix (cpp_string string, char delim, char *suffix) > +{ > + unsigned int len = string.len; > + const char *text = (const char *)string.text; > + int i; > + for (i = len; i > 0; --i) > + { > + if (text[i - 1] == delim) > + break; > + } > + strncpy (suffix, text + i, len - i); > + suffix[len - i] = '\0'; > +} Since you're just going to be passing the pointer to get_identifier anyway, we don't need to copy the suffix into the pointer. Just return a pointer to the start of the suffix. > + if (ud_suffix) > + *ud_suffix = xstrdup ((const char *)str); Similarly here. > + const char *curr_suffix = NULL; > + suffix_id = USERDEF_LITERAL_SUFFIX_ID (tok->u.value); > + curr_suffix = IDENTIFIER_POINTER (suffix_id); > + if (have_suffix_p == 0) > + { > + suffix = xstrdup (curr_suffix); > + have_suffix_p = 1; > + } > + else if (have_suffix_p == 1 && strcmp (suffix, curr_suffix) != 0) And here, you can compare the identifiers with ==, since identifiers are unique. > + error ("inconsistent user-defined literal suffixes"); Let's print the suffixes involved. > + error ("%qD has illegal argument list", decl); Use "invalid" rather than "illegal". > + suffix = UDLIT_OP_SUFFIX (DECL_NAME (decl)); > + if (long_long_unsigned_p) > + { > + if (cpp_interpret_int_suffix (suffix, strlen (suffix))) > + warning (0, "integer suffix shadowed by implementation"); > + } > + else if (long_double_p) > + { > + if (cpp_interpret_float_suffix (suffix, strlen (suffix))) > + warning (0, "floating point suffix" > + " shadowed by implementation"); > + } Let's print the suffix as part of the warning. > + if (dname > + && TREE_CODE (dname) == IDENTIFIER_NODE > + && UDLIT_OPER_P (dname) > + && innermost_code != cdk_function > + && ! (ctype && !declspecs->any_specifiers_p)) I think we can drop the last check, as it only applies to constructors. > +/* User-defined literal operator. > +DEF_SIMPLE_OPERATOR ("\"\"", USERDEF_LITERAL_EXPR, "ud", 1) */ Are you planning to use this for something? > +/* Parse a user-defined char constant. Returns a call to a user-defined > + literal operator taking the character as an argument. */ > +static tree > +cp_parser_userdef_char_literal (cp_parser *parser) There should be a blank line after the comment here and before the other new functions, at least in gcc/. > +make_numeric_string(tree value) This function tries to recreate a printed form for a numeric constant, but that's not right; we need the actual source characters, which are likely to be different, especially for floating-point numbers. So we need to get back to the actual spelling of the token here. I notice that later in cp_parser_userdef_numeric_literal when calling the variadic template version you assume that you have the spelling of the token, which is indeed what we want. > +check_literal_operator_args(const_tree decl, Space before (. > + tree const_char_ptr_type_node > + = build_pointer_type(build_type_variant(char_type_node, 1, 0)); > + tree const_wchar_ptr_type_node > + = build_pointer_type(build_type_variant(wchar_type_node, 1, 0)); > + tree const_char16_ptr_type_node > + = build_pointer_type(build_type_variant(char16_type_node, 1, 0)); > + tree const_char32_ptr_type_node > + = build_pointer_type(build_type_variant(char32_type_node, 1, 0)); It seems to me that rather than build these up to compare against, it it would be faster to check for POINTER_TYPE, then check that the pointed-to type is CP_TYPE_CONST_NON_VOLATILE_P, and then compare the TYPE_MAIN_VARIANT to char_type_node, etc. > +++ b/gcc/testsuite/g++.dg/cpp0x/udlit-operator-neg.C > @@ -0,0 +1,16 @@ > +// { dg-options "-std=c++0x" } > + > +// Both of these can't have *both* of these. > + > +int > +operator"" _abc(const char*) > + { > + return 42; > + } > + > +template > + int > + operator"" _abc() > + { > + return 13; > + } Shouldn't this have a dg-error tag somewhere? Jason