On 09/13/2011 09:52 PM, Jason Merrill wrote:
> 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"
Done.
>
>> -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.
Done.
>
>> @@ -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.
Done, I think.
>
>> +/* 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.
Done.
>
>> +             if (ud_suffix)
>> +               *ud_suffix = xstrdup ((const char *)str);
>
> Similarly here.
Done.
>
>> +             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.
Done.
>
>> +                 error ("inconsistent user-defined literal suffixes");
>
> Let's print the suffixes involved.
Done.
>
>> +             error ("%qD has illegal argument list", decl);
>
> Use "invalid" rather than "illegal".
Done.
>
>> +         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.
Done.
>
>> +  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.
Done.
>
>> +/* User-defined literal operator.
>> +DEF_SIMPLE_OPERATOR ("\"\"", USERDEF_LITERAL_EXPR, "ud", 1)  */
>
> Are you planning to use this for something?
Removed.
>
>> +/* 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/.
Done in as many of my places as I could find.
>
>> +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.
Done.  I am now storing the numeric string along with the number and the 
suffix ID for numeric operators.  I'm keeping the numeric value because 
we need it too.
>
>> +check_literal_operator_args(const_tree decl,
>
> Space before (.
Done.
>
>> +      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.
I tried this and couldn't get it to work.  I got a bunch of false 
negatives and then a crash.
-------------------------------------------------------------------------------
-------------------------------------------------------------------------------
>
>> +++ 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<char...>
>> +  int
>> +  operator"" _abc()
>> +  {
>> +    return 13;
>> +  }
>
> Shouldn't this have a dg-error tag somewhere?
I'm working on checking this.  I need to test if both
   operator"" _foo(char*)
and
   template <char...>
     _foo();
are declared and error.  I'l looking over decl.c for this.
If I can't figure that out I'll take out the test.
>
> Jason
>
Attached are the results to date.