diff mbox

[C++0x] implementing forward declarations for enums

Message ID AANLkTikM8LGvkGL8axC13N551n-rFaZTZEuFSRATJJ4K@mail.gmail.com
State New
Headers show

Commit Message

Rodrigo Rivas Sept. 26, 2010, 2:23 p.m. UTC 
> Great!  BTW, please send C++ patches to gcc-patches and CC me so that I see
> them right away; I tend to fall behind on the mailing lists.
Ok, changing to gcc-patches. (The thread history will be messed up, though).

> I'll take a look at this soon.
Please, review this patch instead. It is updated to a newer revision,
and I've changed a few things.
Most notably, I've moved the begin_scope()/end_scope() of scoped enums
into the parsing code. This way it is much easier to ensure that they
are balanced. I'm not sure if they are needed in the template
instantiation, but added it anyway, just in case.
I'm using the TYPE_LANG_FLAG_2 in order to check for the enumerator
lists, I don't know if this is appropriate.

One more question: scoped enumerations call maybe_warn_cpp0x() so they
are accepted in C++98 mode, but with a warning. Should forward
enumerations be accepted also? Currently, I'm accepting them only in
C++0x mode.

Comments, testscases and changelog still missing.

TIA
Rodrigo.

Regards.
Rodrigo.

Comments

Jason Merrill Sept. 29, 2010, 10:20 p.m. UTC | #1 
On 09/26/2010 10:23 AM, Rodrigo Rivas wrote:
> Most notably, I've moved the begin_scope()/end_scope() of scoped enums
> into the parsing code. This way it is much easier to ensure that they
> are balanced. I'm not sure if they are needed in the template
> instantiation, but added it anyway, just in case.

They are needed.

> I'm using the TYPE_LANG_FLAG_2 in order to check for the enumerator
> lists, I don't know if this is appropriate.

That's fine.

> One more question: scoped enumerations call maybe_warn_cpp0x() so they
> are accepted in C++98 mode, but with a warning. Should forward
> enumerations be accepted also? Currently, I'm accepting them only in
> C++0x mode.

I don't have a strong preference unless libstdc++ really wants to use 
them.  But rather than checking cxx_dialect with ==, compare it against 
cxx0x with < or >=.

> +      /* If this enum has value list from a previous declaration, then it has already
> +         been finished. Do not do it again.  */
> +      must_finish = !ENUM_HAS_VALUE_LIST_P (type);

What if you get two opaque declarations in a row?

> +      if (CLASS_TYPE_P (nested_name_specifier))
> +	{
> +	  nested_being_defined = TYPE_BEING_DEFINED (nested_name_specifier);
> +	  TYPE_BEING_DEFINED (nested_name_specifier) = 1;
> +	  push_scope (nested_name_specifier);
> +	}

Hmm, this looks odd, but I guess it's necessary for adding enumerators 
to the class scope after the class is already complete.

> Comments, testscases and changelog still missing.

Yep.  :)

Jason
Rodrigo Rivas Sept. 30, 2010, 10:36 a.m. UTC | #2 
On Thu, Sep 30, 2010 at 12:20 AM, Jason Merrill <jason@redhat.com> wrote:
>> One more question: scoped enumerations call maybe_warn_cpp0x() so they
>> are accepted in C++98 mode, but with a warning. Should forward
>> enumerations be accepted also? Currently, I'm accepting them only in
>> C++0x mode.
>
> I don't have a strong preference unless libstdc++ really wants to use them.
Well, I don't think it'll use opaque enums, so...

>  But rather than checking cxx_dialect with ==, compare it against cxx0x with
> < or >=.
Oh, I just copied the nearest condition I could find. Should I change
the others too?

>> +      /* If this enum has value list from a previous declaration, then it
>> has already
>> +         been finished. Do not do it again.  */
>> +      must_finish = !ENUM_HAS_VALUE_LIST_P (type);
>
> What if you get two opaque declarations in a row?
Yes, finish_enum() is called every time until the value list is
parsed. That doesn't look good, but it doesn't seem to cause problems
either. Anyway I'm rewriting it: my idea is to split finish_enum() in
two functions (finish_enum_value_list() and finish_enum() properly)
and call any of them appropriately.

>> +      if (CLASS_TYPE_P (nested_name_specifier))
>> +       {
>> +         nested_being_defined = TYPE_BEING_DEFINED
>> (nested_name_specifier);
>> +         TYPE_BEING_DEFINED (nested_name_specifier) = 1;
>> +         push_scope (nested_name_specifier);
>> +       }
>
> Hmm, this looks odd, but I guess it's necessary for adding enumerators to
> the class scope after the class is already complete.
Indeed. There is an assertion that ICEs if this flag is not set. Other
than that it is not used, so I've been tempted to just delete it ;)

>> Comments, testscases and changelog still missing.
> Yep.  :)
Just coming...

BTW, there is some code that is getting in my way. It is the call to
lookup_and_check_tag() in start_enum(). The comment says that this is
related to "forward references". AFAIK, forwards enums are a GCC
extension for C, but they are not enabled in C++ (and anyway are
superseded by opaque enums). Could you tell me if this is still in use
or if it is an evolutionary remnant?

Since you don't seem to object too much to the way I wrote this, I'll
prepare a proper patch in a few days' time.

Regards.
Rodrigo.
Jason Merrill Oct. 4, 2010, 2:20 p.m. UTC | #3 
On 09/30/2010 06:36 AM, Rodrigo Rivas wrote:
> On Thu, Sep 30, 2010 at 12:20 AM, Jason Merrill<jason@redhat.com>  wrote:
>>   But rather than checking cxx_dialect with ==, compare it against cxx0x with
>>   < or >=.
> Oh, I just copied the nearest condition I could find. Should I change
> the others too?

As a separate patch, sure.

> Yes, finish_enum() is called every time until the value list is
> parsed. That doesn't look good, but it doesn't seem to cause problems
> either. Anyway I'm rewriting it: my idea is to split finish_enum() in
> two functions (finish_enum_value_list() and finish_enum() properly)
> and call any of them appropriately.

What bits of finish_enum do you actually need for an opaque enum?  Very 
little, it seems to me.

> BTW, there is some code that is getting in my way. It is the call to
> lookup_and_check_tag() in start_enum(). The comment says that this is
> related to "forward references". AFAIK, forwards enums are a GCC
> extension for C, but they are not enabled in C++ (and anyway are
> superseded by opaque enums). Could you tell me if this is still in use
> or if it is an evolutionary remnant?

Sounds like the latter; what happens if you remove it?

Jason
diff mbox

Patch

Index: gcc/cp/decl.c
===================================================================
--- gcc/cp/decl.c	(revision 164631)
+++ gcc/cp/decl.c	(working copy)
@@ -11331,27 +11331,48 @@  xref_basetypes (tree ref, tree base_list)
    may be used to declare the individual values as they are read.  */
 
 tree
-start_enum (tree name, tree underlying_type, bool scoped_enum_p)
+start_enum (tree name, tree enumtype, tree underlying_type, bool scoped_enum_p)
 {
-  tree enumtype;
-
   gcc_assert (TREE_CODE (name) == IDENTIFIER_NODE);
 
+  /* [C++0x dcl.enum]p5: 
+
+    If not explicitly specified, the underlying type of a scoped
+    enumeration type is int.  */
+  if (!underlying_type && scoped_enum_p)
+    underlying_type = integer_type_node;
+
   /* If this is the real definition for a previous forward reference,
      fill in the contents in the same object that used to be the
      forward reference.  */
-
-  enumtype = lookup_and_check_tag (enum_type, name,
-				   /*tag_scope=*/ts_current,
-				   /*template_header_p=*/false);
-
+  if (!enumtype)
+    enumtype = lookup_and_check_tag (enum_type, name,
+				     /*tag_scope=*/ts_current,
+				     /*template_header_p=*/false);
   if (enumtype != NULL_TREE && TREE_CODE (enumtype) == ENUMERAL_TYPE)
     {
-      error_at (input_location, "multiple definition of %q#T", enumtype);
-      error_at (DECL_SOURCE_LOCATION (TYPE_MAIN_DECL (enumtype)),
-		"previous definition here");
-      /* Clear out TYPE_VALUES, and start again.  */
-      TYPE_VALUES (enumtype) = NULL_TREE;
+      if (scoped_enum_p != SCOPED_ENUM_P (enumtype))
+	{
+	  error_at (input_location, "scoped/unscoped mismatch in enum %q#T", enumtype);
+	  error_at (DECL_SOURCE_LOCATION (TYPE_MAIN_DECL (enumtype)),
+		    "previous definition here");
+	}
+      if (!underlying_type
+	  || !same_type_p (underlying_type, ENUM_UNDERLYING_TYPE (enumtype)))
+	{
+	  error_at (input_location, "different underlying type in enum %q#T", enumtype);
+	  error_at (DECL_SOURCE_LOCATION (TYPE_MAIN_DECL (enumtype)),
+		    "previous definition here");
+	}
+
+      if (cxx_dialect == cxx98)
+	{
+	  error_at (input_location, "multiple definition of %q#T", enumtype);
+	  error_at (DECL_SOURCE_LOCATION (TYPE_MAIN_DECL (enumtype)),
+		    "previous definition here");
+	  /* Clear out TYPE_VALUES, and start again.  */
+	  TYPE_VALUES (enumtype) = NULL_TREE;
+	}
     }
   else
     {
@@ -11367,19 +11388,8 @@  tree
   if (enumtype == error_mark_node)
     return enumtype;
 
-  if (scoped_enum_p)
-    {
-      SET_SCOPED_ENUM_P (enumtype, 1);
-      begin_scope (sk_scoped_enum, enumtype);
+  SET_SCOPED_ENUM_P (enumtype, scoped_enum_p);
 
-      /* [C++0x dcl.enum]p5: 
-
-          If not explicitly specified, the underlying type of a scoped
-          enumeration type is int.  */
-      if (!underlying_type)
-        underlying_type = integer_type_node;
-    }
-
   if (underlying_type)
     {
       if (CP_INTEGRAL_TYPE_P (underlying_type))
@@ -11435,8 +11445,6 @@  finish_enum (tree enumtype)
 	TREE_TYPE (TREE_VALUE (values)) = enumtype;
       if (at_function_scope_p ())
 	add_stmt (build_min (TAG_DEFN, enumtype));
-      if (SCOPED_ENUM_P (enumtype))
-	finish_scope ();
       return;
     }
 
@@ -11625,10 +11633,6 @@  finish_enum (tree enumtype)
       ENUM_UNDERLYING_TYPE (t) = ENUM_UNDERLYING_TYPE (enumtype);
     }
 
-  /* Finish up the scope of a scoped enumeration.  */
-  if (SCOPED_ENUM_P (enumtype))
-    finish_scope ();
-
   /* Finish debugging output for this type.  */
   rest_of_type_compilation (enumtype, namespace_bindings_p ());
 }
Index: gcc/cp/pt.c
===================================================================
--- gcc/cp/pt.c	(revision 164631)
+++ gcc/cp/pt.c	(working copy)
@@ -6654,7 +6654,7 @@  lookup_template_class (tree d1,
 	  if (!is_dependent_type)
 	    {
 	      set_current_access_from_decl (TYPE_NAME (template_type));
-	      t = start_enum (TYPE_IDENTIFIER (template_type),
+	      t = start_enum (TYPE_IDENTIFIER (template_type), NULL_TREE,
                               tsubst (ENUM_UNDERLYING_TYPE (template_type),
                                       arglist, complain, in_decl),
                               SCOPED_ENUM_P (template_type));
@@ -17292,6 +17292,9 @@  static void
 tsubst_enum (tree tag, tree newtag, tree args)
 {
   tree e;
+  
+  if (SCOPED_ENUM_P (newtag))
+    begin_scope (sk_scoped_enum, newtag);
 
   for (e = TYPE_VALUES (tag); e; e = TREE_CHAIN (e))
     {
@@ -17313,6 +17316,9 @@  tsubst_enum (tree tag, tree newtag, tree args)
 	(DECL_NAME (decl), value, newtag, DECL_SOURCE_LOCATION (decl));
     }
 
+  if (SCOPED_ENUM_P (newtag))
+    finish_scope ();
+
   finish_enum (newtag);
   DECL_SOURCE_LOCATION (TYPE_NAME (newtag))
     = DECL_SOURCE_LOCATION (TYPE_NAME (tag));
Index: gcc/cp/parser.c
===================================================================
--- gcc/cp/parser.c	(revision 164631)
+++ gcc/cp/parser.c	(working copy)
@@ -13212,10 +13204,13 @@  static tree
 cp_parser_enum_specifier (cp_parser* parser)
 {
   tree identifier;
-  tree type;
+  tree type = NULL_TREE;
+  tree nested_name_specifier;
   tree attributes;
   bool scoped_enum_p = false;
   bool has_underlying_type = false;
+  bool nested_being_defined = false;
+  bool must_finish = true;
   tree underlying_type = NULL_TREE;
 
   /* Parse tentatively so that we can back up if we don't find a
@@ -13244,10 +13239,36 @@  cp_parser_enum_specifier (cp_parser* parser)
 
   attributes = cp_parser_attributes_opt (parser);
 
-  if (cp_lexer_next_token_is (parser->lexer, CPP_NAME))
-    identifier = cp_parser_identifier (parser);
+  push_deferring_access_checks (dk_no_check);
+  nested_name_specifier
+    = cp_parser_nested_name_specifier_opt (parser,
+					   /*typename_keyword_p=*/false,
+					   /*check_dependency_p=*/false,
+					   /*type_p=*/false,
+					   /*is_declaration=*/false);
+
+  if (nested_name_specifier)
+    {
+      tree name;
+      identifier = cp_parser_identifier (parser);
+      name =  cp_parser_lookup_name (parser, identifier,
+				     enum_type,
+				     /*is_template=*/false,
+				     /*is_namespace=*/false,
+				     /*check_dependency=*/true,
+				     /*ambiguous_decls=*/NULL,
+				     input_location);
+      if (name)
+	type = TREE_TYPE (name);
+    }
   else
-    identifier = make_anon_name ();
+    {
+      if (cp_lexer_next_token_is (parser->lexer, CPP_NAME))
+	identifier = cp_parser_identifier (parser);
+      else
+	identifier = make_anon_name ();
+    }
+  pop_deferring_access_checks ();
 
   /* Check for the `:' that denotes a specified underlying type in C++0x.
      Note that a ':' could also indicate a bitfield width, however.  */
@@ -13285,14 +13306,31 @@  cp_parser_enum_specifier (cp_parser* parser)
   /* Look for the `{' but don't consume it yet.  */
   if (!cp_lexer_next_token_is (parser->lexer, CPP_OPEN_BRACE))
     {
-      cp_parser_error (parser, "expected %<{%>");
-      if (has_underlying_type)
-	return NULL_TREE;
+      if (cxx_dialect == cxx98 || (!scoped_enum_p && !underlying_type))
+	{
+	  cp_parser_error (parser, "expected %<{%>");
+	  if (has_underlying_type)
+	    return NULL_TREE;
+	}
     }
 
   if (!has_underlying_type && !cp_parser_parse_definitely (parser))
     return NULL_TREE;
 
+  if (nested_name_specifier && !scoped_enum_p)
+    {
+      if (CLASS_TYPE_P (nested_name_specifier))
+	{
+	  nested_being_defined = TYPE_BEING_DEFINED (nested_name_specifier);
+	  TYPE_BEING_DEFINED (nested_name_specifier) = 1;
+	  push_scope (nested_name_specifier);
+	}
+      else if ( TREE_CODE(nested_name_specifier) == NAMESPACE_DECL)
+	{
+	  push_nested_namespace (nested_name_specifier);
+	}
+    }
+
   /* Issue an error message if type-definitions are forbidden here.  */
   if (!cp_parser_check_type_definition (parser))
     type = error_mark_node;
@@ -13300,24 +13338,49 @@  cp_parser_enum_specifier (cp_parser* parser)
     /* Create the new type.  We do this before consuming the opening
        brace so the enum will be recorded as being on the line of its
        tag (or the 'enum' keyword, if there is no tag).  */
-    type = start_enum (identifier, underlying_type, scoped_enum_p);
-  
-  /* Consume the opening brace.  */
-  cp_lexer_consume_token (parser->lexer);
+    type = start_enum (identifier, type, underlying_type, scoped_enum_p);
 
-  if (type == error_mark_node)
+  /* If the next token is not '{' it is an opaque enum.  */
+  if (cp_lexer_next_token_is (parser->lexer, CPP_OPEN_BRACE))
     {
-      cp_parser_skip_to_end_of_block_or_statement (parser);
-      return error_mark_node;
-    }
+      if (scoped_enum_p)
+	begin_scope (sk_scoped_enum, type);
 
-  /* If the next token is not '}', then there are some enumerators.  */
-  if (cp_lexer_next_token_is_not (parser->lexer, CPP_CLOSE_BRACE))
-    cp_parser_enumerator_list (parser, type);
+      /* Consume the opening brace.  */
+      cp_lexer_consume_token (parser->lexer);
 
-  /* Consume the final '}'.  */
-  cp_parser_require (parser, CPP_CLOSE_BRACE, RT_CLOSE_BRACE);
+      if (ENUM_HAS_VALUE_LIST_P (type))
+	{
+	  error_at (input_location, "multiple definition of %q#T", type);
+	  error_at (DECL_SOURCE_LOCATION (TYPE_MAIN_DECL (type)),
+		    "previous definition here");
+	  type = error_mark_node;
+	}
+      else
+	ENUM_HAS_VALUE_LIST_P (type) = true;
 
+      if (type == error_mark_node)
+	{
+	  cp_parser_skip_to_end_of_block_or_statement (parser);
+	  must_finish = false;
+	}
+      /* If the next token is not '}', then there are some enumerators.  */
+      else if (cp_lexer_next_token_is_not (parser->lexer, CPP_CLOSE_BRACE))
+	cp_parser_enumerator_list (parser, type);
+
+      /* Consume the final '}'.  */
+      cp_parser_require (parser, CPP_CLOSE_BRACE, RT_CLOSE_BRACE);
+
+      if (scoped_enum_p)
+	finish_scope ();
+    }
+  else
+    {
+      /* If this enum has value list from a previous declaration, then it has already
+         been finished. Do not do it again.  */
+      must_finish = !ENUM_HAS_VALUE_LIST_P (type);
+    }
+
   /* Look for trailing attributes to apply to this enumeration, and
      apply them if appropriate.  */
   if (cp_parser_allow_gnu_extensions_p (parser))
@@ -13330,8 +13393,21 @@  cp_parser_enum_specifier (cp_parser* parser)
     }
 
   /* Finish up the enumeration.  */
-  finish_enum (type);
+  if (must_finish)
+    finish_enum (type);
 
+  if (nested_name_specifier && !scoped_enum_p)
+    {
+      if (CLASS_TYPE_P (nested_name_specifier))
+	{
+	  TYPE_BEING_DEFINED (nested_name_specifier) = nested_being_defined;
+	  pop_scope (nested_name_specifier);
+	}
+      else if ( TREE_CODE(nested_name_specifier) == NAMESPACE_DECL)
+	{
+	  pop_nested_namespace (nested_name_specifier);
+	}
+    }
   return type;
 }
 
Index: gcc/cp/cp-tree.h
===================================================================
--- gcc/cp/cp-tree.h	(revision 164631)
+++ gcc/cp/cp-tree.h	(working copy)
@@ -3102,6 +3102,8 @@  more_aggr_init_expr_args_p (const aggr_init_expr_a
 #define SET_SCOPED_ENUM_P(TYPE, VAL)                    \
   (ENUM_IS_SCOPED (TYPE) = (VAL))
 
+#define ENUM_HAS_VALUE_LIST_P(NODE) (TYPE_LANG_FLAG_2 (NODE))
+
 /* Returns the underlying type of the given enumeration type. The
    underlying type is determined in different ways, depending on the
    properties of the enum:
@@ -4778,7 +4780,7 @@  extern bool grok_op_properties			(tree, bool);
 extern tree xref_tag				(enum tag_types, tree, tag_scope, bool);
 extern tree xref_tag_from_type			(tree, tree, tag_scope);
 extern bool xref_basetypes			(tree, tree);
-extern tree start_enum				(tree, tree, bool);
+extern tree start_enum				(tree, tree, tree, bool);
 extern void finish_enum				(tree);
 extern void build_enumerator			(tree, tree, tree, location_t);
 extern tree lookup_enumerator			(tree, tree);