diff mbox

[RFC] Implement N4230, Nested namespace definition

Message ID CAFk2RUbz+z9pC8JcLWFtqMMWEWdu5M8uRyoW-nygyVF3fz2J9g@mail.gmail.com
State New
Headers show

Commit Message

Ville Voutilainen Sept. 17, 2015, 10:23 p.m. UTC 
On 17 September 2015 at 23:11, Jason Merrill <jason@redhat.com> wrote:
> On 09/16/2015 07:55 AM, Ville Voutilainen wrote:
>>
>> This is the first stab, I haven't written the tests yet. Feedback would be
>> most welcome; should I put this code into a separate function? Is the
>> minor
>> code duplication with the regular namespace definition ok?
>
>
> I think I'd prefer to keep it in the same function, but avoid the code
> duplication.

Ok. Tested on Linux-PPC64. This patch doesn't handle attributes yet, it looks to
me as if gcc doesn't support namespace attributes in the location that
the standard
grammar puts them into. I had to adjust a couple of
point-of-declaration error/warning
locations in the testsuite, but those seem reasonable to me, biased as
I may be towards keeping
this patch simpler than keeping those locations where they were would
likely require.

Nathan, this patch touches areas close to ones that your "Coding rule
enforcement" patch
does, please be aware of potential merge conflicts.

/cp
2015-09-18  Ville Voutilainen  <ville.voutilainen@gmail.com>

    Implement nested namespace definitions.
    * parser.c (cp_parser_namespace_definition): Grok nested namespace
    definitions.

/testsuite
2015-09-18  Ville Voutilainen  <ville.voutilainen@gmail.com>

    Implement nested namespace definitions.
    * g++.dg/cpp1z/nested-namespace-def.C: New.
    * g++.dg/lookup/name-clash5.C: Adjust.
    * g++.dg/lookup/name-clash6.C: Likewise.

Comments

Ville Voutilainen Sept. 17, 2015, 11:02 p.m. UTC | #1 
On 18 September 2015 at 01:23, Ville Voutilainen
<ville.voutilainen@gmail.com> wrote:
> Ok. Tested on Linux-PPC64. This patch doesn't handle attributes yet, it looks to
> me as if gcc doesn't support namespace attributes in the location that
> the standard
> grammar puts them into. I had to adjust a couple of


Ahem, oops, the patch doesn't do any sort of a pedwarn for standard versions
below cpp1z; I'll do a new patch taking that into account tomorrow. I don't
think we have maybe_warn_cpp1z or anything like that? Any preferences
how to deal with that?
Jason Merrill Sept. 18, 2015, 4:27 p.m. UTC | #2 
On 09/17/2015 06:23 PM, Ville Voutilainen wrote:
> This patch doesn't handle attributes yet, it looks to
> me as if gcc doesn't support namespace attributes in the location that
> the standard grammar puts them into.

Mind fixing that, too?

> +                 "-std=c++17 or -std=gnu++17");

Please use "1z" until C++17 is final.

Jason
Ville Voutilainen Sept. 18, 2015, 4:30 p.m. UTC | #3 
On 18 September 2015 at 19:27, Jason Merrill <jason@redhat.com> wrote:
> On 09/17/2015 06:23 PM, Ville Voutilainen wrote:
>>
>> This patch doesn't handle attributes yet, it looks to
>> me as if gcc doesn't support namespace attributes in the location that
>> the standard grammar puts them into.
> Mind fixing that, too?

Can we please do that separately?

>
>> +                 "-std=c++17 or -std=gnu++17");
> Please use "1z" until C++17 is final.


Will do.
Jason Merrill Sept. 18, 2015, 4:34 p.m. UTC | #4 
On 09/18/2015 12:30 PM, Ville Voutilainen wrote:
> On 18 September 2015 at 19:27, Jason Merrill <jason@redhat.com> wrote:
>> On 09/17/2015 06:23 PM, Ville Voutilainen wrote:
>>>
>>> This patch doesn't handle attributes yet, it looks to
>>> me as if gcc doesn't support namespace attributes in the location that
>>> the standard grammar puts them into.
>> Mind fixing that, too?
>
> Can we please do that separately?

I suppose so, but it seems pretty trivial.  In any case, looks like your 
patch would accept the odd

namespace A __attribute ((visibility ("default"))) ::B { }

Jason
Ville Voutilainen Sept. 18, 2015, 4:58 p.m. UTC | #5 
On 18 September 2015 at 19:34, Jason Merrill <jason@redhat.com> wrote:
>>>> This patch doesn't handle attributes yet, it looks to
>>>> me as if gcc doesn't support namespace attributes in the location that
>>>> the standard grammar puts them into.
>>> Mind fixing that, too?
>> Can we please do that separately?
> I suppose so, but it seems pretty trivial.  In any case, looks like your
> patch would accept the odd
> namespace A __attribute ((visibility ("default"))) ::B { }


Yes, or namespace A[[nonsense]]::B {}. Those cases are easy to fix,
but namespace [[attribute_in_proper_location]] A {} seemingly caused
weird barfing. That's why I didn't put in the rejection of the former,
I'd prefer
to figure out the latter and the former at the same time, and I'd prefer doing
that once the basic facility is in. Yes, partly because I'll travel tomorrow. :)
Jason Merrill Sept. 18, 2015, 5:26 p.m. UTC | #6 
On 09/18/2015 12:58 PM, Ville Voutilainen wrote:
> On 18 September 2015 at 19:34, Jason Merrill <jason@redhat.com> wrote:
>>>>> This patch doesn't handle attributes yet, it looks to
>>>>> me as if gcc doesn't support namespace attributes in the location that
>>>>> the standard grammar puts them into.
>>>> Mind fixing that, too?
>>> Can we please do that separately?
>> I suppose so, but it seems pretty trivial.  In any case, looks like your
>> patch would accept the odd
>> namespace A __attribute ((visibility ("default"))) ::B { }
>
> Yes, or namespace A[[nonsense]]::B {}. Those cases are easy to fix,
> but namespace [[attribute_in_proper_location]] A {} seemingly caused
> weird barfing. That's why I didn't put in the rejection of the former, I'd prefer
> to figure out the latter and the former at the same time, and I'd prefer doing
> that once the basic facility is in. Yes, partly because I'll travel tomorrow. :)

To fix the former, you just need to keep

>    /* Parse any specified attributes.  */
>    attribs = cp_parser_attributes_opt (parser);

next to the open brace.  OK with that change, I suppose the other can wait.

Jason
Ville Voutilainen Sept. 18, 2015, 5:30 p.m. UTC | #7 
On 18 September 2015 at 20:26, Jason Merrill <jason@redhat.com> wrote:
>>> I suppose so, but it seems pretty trivial.  In any case, looks like your
>>> patch would accept the odd
>>> namespace A __attribute ((visibility ("default"))) ::B { }
>> Yes, or namespace A[[nonsense]]::B {}. Those cases are easy to fix,
>> but namespace [[attribute_in_proper_location]] A {} seemingly caused
>> weird barfing. That's why I didn't put in the rejection of the former, I'd
>> prefer
>> to figure out the latter and the former at the same time, and I'd prefer
>> doing
>> that once the basic facility is in. Yes, partly because I'll travel
>> tomorrow. :)
> To fix the former, you just need to keep
>>    /* Parse any specified attributes.  */
>>    attribs = cp_parser_attributes_opt (parser);
> next to the open brace.  OK with that change, I suppose the other can wait.


I also need to diagnose the use of attributes with a nested namespace
definition,
so I need to add the error emission and test it. ;)
Ville Voutilainen Sept. 18, 2015, 5:38 p.m. UTC | #8 
On 18 September 2015 at 20:30, Ville Voutilainen
<ville.voutilainen@gmail.com> wrote:
> On 18 September 2015 at 20:26, Jason Merrill <jason@redhat.com> wrote:
>>>> I suppose so, but it seems pretty trivial.  In any case, looks like your
>>>> patch would accept the odd
>>>> namespace A __attribute ((visibility ("default"))) ::B { }
>>> Yes, or namespace A[[nonsense]]::B {}. Those cases are easy to fix,
>>> but namespace [[attribute_in_proper_location]] A {} seemingly caused
>>> weird barfing. That's why I didn't put in the rejection of the former, I'd
>>> prefer
>>> to figure out the latter and the former at the same time, and I'd prefer
>>> doing
>>> that once the basic facility is in. Yes, partly because I'll travel
>>> tomorrow. :)
>> To fix the former, you just need to keep
>>>    /* Parse any specified attributes.  */
>>>    attribs = cp_parser_attributes_opt (parser);
>> next to the open brace.  OK with that change, I suppose the other can wait.
>
>
> I also need to diagnose the use of attributes with a nested namespace
> definition,
> so I need to add the error emission and test it. ;)

Hmm, I already do that, the nested namespace definition parsing
effectively requires
an identifier. Ok, I'll give it a spin, I'll send an updated patch for
review. :)
Ville Voutilainen Sept. 18, 2015, 5:46 p.m. UTC | #9 
On 18 September 2015 at 20:38, Ville Voutilainen
<ville.voutilainen@gmail.com> wrote:
> On 18 September 2015 at 20:30, Ville Voutilainen
> <ville.voutilainen@gmail.com> wrote:
>> On 18 September 2015 at 20:26, Jason Merrill <jason@redhat.com> wrote:
>>>>> I suppose so, but it seems pretty trivial.  In any case, looks like your
>>>>> patch would accept the odd
>>>>> namespace A __attribute ((visibility ("default"))) ::B { }
>>>> Yes, or namespace A[[nonsense]]::B {}. Those cases are easy to fix,
>>>> but namespace [[attribute_in_proper_location]] A {} seemingly caused
>>>> weird barfing. That's why I didn't put in the rejection of the former, I'd
>>>> prefer
>>>> to figure out the latter and the former at the same time, and I'd prefer
>>>> doing
>>>> that once the basic facility is in. Yes, partly because I'll travel
>>>> tomorrow. :)
>>> To fix the former, you just need to keep
>>>>    /* Parse any specified attributes.  */
>>>>    attribs = cp_parser_attributes_opt (parser);
>>> next to the open brace.  OK with that change, I suppose the other can wait.
>>
>>
>> I also need to diagnose the use of attributes with a nested namespace
>> definition,
>> so I need to add the error emission and test it. ;)
>
> Hmm, I already do that, the nested namespace definition parsing
> effectively requires
> an identifier. Ok, I'll give it a spin, I'll send an updated patch for
> review. :)

Argh, no. An attribute immediately following a nesting namespace would need
to be parsed before the nested namespace definition handling is done, otherwise
the nested namespace definition handling is never entered because the next token
is not CPP_SCOPE. So the attributes should be parsed and rejected where they are
parsed now if they are followed by a CPP_SCOPE. That's easy, I'll just check
for non-null attribs and diagnose.
diff mbox

Patch

diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index 4f424b6..9fee310 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -16953,6 +16953,8 @@  cp_parser_namespace_definition (cp_parser* parser)
   tree identifier, attribs;
   bool has_visibility;
   bool is_inline;
+  cp_token* token;
+  int nested_definition_count = 0;
 
   cp_ensure_no_omp_declare_simd (parser);
   if (cp_lexer_next_token_is_keyword (parser->lexer, RID_INLINE))
@@ -16965,7 +16967,7 @@  cp_parser_namespace_definition (cp_parser* parser)
     is_inline = false;
 
   /* Look for the `namespace' keyword.  */
-  cp_parser_require_keyword (parser, RID_NAMESPACE, RT_NAMESPACE);
+  token = cp_parser_require_keyword (parser, RID_NAMESPACE, RT_NAMESPACE);
 
   /* Get the name of the namespace.  We do not attempt to distinguish
      between an original-namespace-definition and an
@@ -16979,11 +16981,32 @@  cp_parser_namespace_definition (cp_parser* parser)
   /* Parse any specified attributes.  */
   attribs = cp_parser_attributes_opt (parser);
 
-  /* Look for the `{' to start the namespace.  */
-  cp_parser_require (parser, CPP_OPEN_BRACE, RT_OPEN_BRACE);
   /* Start the namespace.  */
   push_namespace (identifier);
 
+  /* Parse any nested namespace definition. */
+  if (cp_lexer_next_token_is (parser->lexer, CPP_SCOPE))
+    {
+      if (is_inline)
+        error_at (token->location, "a nested %<namespace%> definition cannot be inline");
+      while (cp_lexer_next_token_is (parser->lexer, CPP_SCOPE))
+        {
+          cp_lexer_consume_token (parser->lexer);
+          if (cp_lexer_next_token_is (parser->lexer, CPP_NAME))
+            identifier = cp_parser_identifier (parser);
+          else
+            {
+              cp_parser_error (parser, "nested identifier required");
+              break;
+            }
+          ++nested_definition_count;
+          push_namespace (identifier);
+        }
+    }
+
+  /* Look for the `{' to validate starting the namespace.  */
+  cp_parser_require (parser, CPP_OPEN_BRACE, RT_OPEN_BRACE);
+
   /* "inline namespace" is equivalent to a stub namespace definition
      followed by a strong using directive.  */
   if (is_inline)
@@ -17007,6 +17030,10 @@  cp_parser_namespace_definition (cp_parser* parser)
   if (has_visibility)
     pop_visibility (1);
 
+  /* Finish the nested namespace definitions.  */
+  while (nested_definition_count--)
+    pop_namespace ();
+
   /* Finish the namespace.  */
   pop_namespace ();
   /* Look for the final `}'.  */
diff --git a/gcc/testsuite/g++.dg/cpp1z/nested-namespace-def.C b/gcc/testsuite/g++.dg/cpp1z/nested-namespace-def.C
new file mode 100644
index 0000000..da35835
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1z/nested-namespace-def.C
@@ -0,0 +1,14 @@ 
+// { dg-options "-std=c++1z" }
+
+namespace A::B::C
+{
+	struct X {};
+	namespace T::U::V { struct Y {}; };
+};
+
+A::B::C::X x;
+A::B::C::T::U::V::Y y;
+
+inline namespace D::E {}; // { dg-error "cannot be inline" }
+
+namespace F::G:: {}; // { dg-error "nested identifier required" }
diff --git a/gcc/testsuite/g++.dg/lookup/name-clash5.C b/gcc/testsuite/g++.dg/lookup/name-clash5.C
index 74595c2..9673bb9 100644
--- a/gcc/testsuite/g++.dg/lookup/name-clash5.C
+++ b/gcc/testsuite/g++.dg/lookup/name-clash5.C
@@ -6,8 +6,8 @@ 
 // "[Note: a namespace name or a class template name must be unique in its
 // declarative region (7.3.2, clause 14). ]"
 
-namespace N
-{ // { dg-message "previous declaration" }
+namespace N // { dg-message "previous declaration" }
+{
 }
 
 class N; // { dg-error "redeclared" }
diff --git a/gcc/testsuite/g++.dg/lookup/name-clash6.C b/gcc/testsuite/g++.dg/lookup/name-clash6.C
index 6918142..f27e04a 100644
--- a/gcc/testsuite/g++.dg/lookup/name-clash6.C
+++ b/gcc/testsuite/g++.dg/lookup/name-clash6.C
@@ -8,6 +8,6 @@ 
 
 class N; // { dg-message "previous declaration" }
 
-namespace N
-{ // { dg-error "redeclared" }
+namespace N // { dg-error "redeclared" }
+{
 }