| Message ID | 20181119212119.GX28582@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | C++ PATCH to implement P1094R2, Nested inline namespaces | expand |
On Mon, Nov 19, 2018 at 04:21:19PM -0500, Marek Polacek wrote: > 2018-11-19 Marek Polacek <polacek@redhat.com> > > Implement P1094R2, Nested inline namespaces. > * g++.dg/cpp2a/nested-inline-ns1.C: New test. > * g++.dg/cpp2a/nested-inline-ns2.C: New test. > * g++.dg/cpp2a/nested-inline-ns3.C: New test. Just a small testsuite comment. > --- /dev/null > +++ gcc/testsuite/g++.dg/cpp2a/nested-inline-ns1.C > @@ -0,0 +1,26 @@ > +// P1094R2 > +// { dg-do compile { target c++2a } } Especially because 2a testing isn't included by default, but also to make sure it works right even with -std=c++17, wouldn't it be better to drop the nested-inline-ns3.C test, make this test c++17 or even better always enabled, add dg-options "-Wpedantic" and just add dg-warning with c++17_down and c++14_down what should be warned on the 3 lines (with .-1 for c++14_down)? Or if you want add some further testcases that will test how c++17 etc. will dg-error on those with -pedantic-errors etc. > + > +namespace A::inline B::C { > + int i; > +} > + > +namespace D::E::inline F { > + int j; > +} > + > +inline namespace X { > + int x; > +} > +// Make sure the namespaces are marked inline. > +void > +g () > +{ > + A::B::C::i++; > + A::C::i++; > + D::E::j++; > + D::E::F::j++; > + X::x++; > + x++; > +} Jakub
On Mon, Nov 19, 2018 at 10:33:17PM +0100, Jakub Jelinek wrote: > On Mon, Nov 19, 2018 at 04:21:19PM -0500, Marek Polacek wrote: > > 2018-11-19 Marek Polacek <polacek@redhat.com> > > > > Implement P1094R2, Nested inline namespaces. > > * g++.dg/cpp2a/nested-inline-ns1.C: New test. > > * g++.dg/cpp2a/nested-inline-ns2.C: New test. > > * g++.dg/cpp2a/nested-inline-ns3.C: New test. > > Just a small testsuite comment. > > > --- /dev/null > > +++ gcc/testsuite/g++.dg/cpp2a/nested-inline-ns1.C > > @@ -0,0 +1,26 @@ > > +// P1094R2 > > +// { dg-do compile { target c++2a } } > > Especially because 2a testing isn't included by default, but also > to make sure it works right even with -std=c++17, wouldn't it be better to > drop the nested-inline-ns3.C test, make this test c++17 or > even better always enabled, add dg-options "-Wpedantic" and > just add dg-warning with c++17_down and c++14_down what should be > warned on the 3 lines (with .-1 for c++14_down)? > > Or if you want add some further testcases that will test how > c++17 etc. will dg-error on those with -pedantic-errors etc. Sure, I've made it { target c++11 } and dropped the third test: Bootstrapped/regtested on x86_64-linux, ok for trunk? 2018-11-19 Marek Polacek <polacek@redhat.com> Implement P1094R2, Nested inline namespaces. * parser.c (cp_parser_namespace_definition): Parse the optional inline keyword in a nested-namespace-definition. Adjust push_namespace call. Formatting fix. * g++.dg/cpp2a/nested-inline-ns1.C: New test. * g++.dg/cpp2a/nested-inline-ns2.C: New test. diff --git gcc/cp/parser.c gcc/cp/parser.c index 292cce15676..f39e9d753d2 100644 --- gcc/cp/parser.c +++ gcc/cp/parser.c @@ -18872,6 +18872,7 @@ cp_parser_namespace_definition (cp_parser* parser) cp_ensure_no_oacc_routine (parser); bool is_inline = cp_lexer_next_token_is_keyword (parser->lexer, RID_INLINE); + const bool topmost_inline_p = is_inline; if (is_inline) { @@ -18890,6 +18891,17 @@ cp_parser_namespace_definition (cp_parser* parser) { identifier = NULL_TREE; + bool nested_inline_p = cp_lexer_next_token_is_keyword (parser->lexer, + RID_INLINE); + if (nested_inline_p && nested_definition_count != 0) + { + if (cxx_dialect < cxx2a) + pedwarn (cp_lexer_peek_token (parser->lexer)->location, + OPT_Wpedantic, "nested inline namespace definitions only " + "available with -std=c++2a or -std=gnu++2a"); + cp_lexer_consume_token (parser->lexer); + } + if (cp_lexer_next_token_is (parser->lexer, CPP_NAME)) { identifier = cp_parser_identifier (parser); @@ -18904,7 +18916,12 @@ cp_parser_namespace_definition (cp_parser* parser) } if (cp_lexer_next_token_is_not (parser->lexer, CPP_SCOPE)) - break; + { + /* Don't forget that the innermost namespace might have been + marked as inline. */ + is_inline |= nested_inline_p; + break; + } if (!nested_definition_count && cxx_dialect < cxx17) pedwarn (input_location, OPT_Wpedantic, @@ -18913,7 +18930,9 @@ cp_parser_namespace_definition (cp_parser* parser) /* Nested namespace names can create new namespaces (unlike other qualified-ids). */ - if (int count = identifier ? push_namespace (identifier) : 0) + if (int count = (identifier + ? push_namespace (identifier, nested_inline_p) + : 0)) nested_definition_count += count; else cp_parser_error (parser, "nested namespace name required"); @@ -18926,7 +18945,7 @@ cp_parser_namespace_definition (cp_parser* parser) if (nested_definition_count && attribs) error_at (token->location, "a nested namespace definition cannot have attributes"); - if (nested_definition_count && is_inline) + if (nested_definition_count && topmost_inline_p) error_at (token->location, "a nested namespace definition cannot be inline"); @@ -18935,7 +18954,7 @@ cp_parser_namespace_definition (cp_parser* parser) bool has_visibility = handle_namespace_attrs (current_namespace, attribs); - warning (OPT_Wnamespaces, "namespace %qD entered", current_namespace); + warning (OPT_Wnamespaces, "namespace %qD entered", current_namespace); /* Look for the `{' to validate starting the namespace. */ matching_braces braces; diff --git gcc/testsuite/g++.dg/cpp2a/nested-inline-ns1.C gcc/testsuite/g++.dg/cpp2a/nested-inline-ns1.C new file mode 100644 index 00000000000..8c9573ea5db --- /dev/null +++ gcc/testsuite/g++.dg/cpp2a/nested-inline-ns1.C @@ -0,0 +1,29 @@ +// P1094R2 +// { dg-do compile { target c++11 } } +// { dg-options "-Wpedantic" } + +namespace A::inline B::C { // { dg-warning "nested inline namespace definitions only" "" { target c++17_down } } +// { dg-warning "nested namespace definitions only available" "" { target c++14_down } .-1 } + int i; +} + +namespace D::E::inline F { // { dg-warning "nested inline namespace definitions only" "" { target c++17_down } } +// { dg-warning "nested namespace definitions only available" "" { target c++14_down } .-1 } + int j; +} + +inline namespace X { + int x; +} + +// Make sure the namespaces are marked inline. +void +g () +{ + A::B::C::i++; + A::C::i++; + D::E::j++; + D::E::F::j++; + X::x++; + x++; +} diff --git gcc/testsuite/g++.dg/cpp2a/nested-inline-ns2.C gcc/testsuite/g++.dg/cpp2a/nested-inline-ns2.C new file mode 100644 index 00000000000..9b5f2cab47b --- /dev/null +++ gcc/testsuite/g++.dg/cpp2a/nested-inline-ns2.C @@ -0,0 +1,26 @@ +// P1094R2 +// { dg-do compile { target c++2a } } + +inline namespace A::B { // { dg-error "a nested namespace definition cannot be inline" } + int i; +} + +namespace inline C::D { // { dg-error "expected|does not name a type" } + int i; +} + +namespace E::F inline { // { dg-error "expected" } + int i; +} + +namespace inline G { // { dg-error "expected|does not name a type" } + int i; +} + +inline namespace inline H { // { dg-error "expected|does not name a type" } + int i; +} + +inline namespace inline I::J { // { dg-error "expected|does not name a type" } + int i; +}
On Mon, Nov 19, 2018 at 11:12 PM Marek Polacek <polacek@redhat.com> wrote: > > On Mon, Nov 19, 2018 at 10:33:17PM +0100, Jakub Jelinek wrote: > > On Mon, Nov 19, 2018 at 04:21:19PM -0500, Marek Polacek wrote: > > > 2018-11-19 Marek Polacek <polacek@redhat.com> > > > > > > Implement P1094R2, Nested inline namespaces. > > > * g++.dg/cpp2a/nested-inline-ns1.C: New test. > > > * g++.dg/cpp2a/nested-inline-ns2.C: New test. > > > * g++.dg/cpp2a/nested-inline-ns3.C: New test. > > > > Just a small testsuite comment. > > > > > --- /dev/null > > > +++ gcc/testsuite/g++.dg/cpp2a/nested-inline-ns1.C > > > @@ -0,0 +1,26 @@ > > > +// P1094R2 > > > +// { dg-do compile { target c++2a } } > > > > Especially because 2a testing isn't included by default, but also > > to make sure it works right even with -std=c++17, wouldn't it be better to > > drop the nested-inline-ns3.C test, make this test c++17 or > > even better always enabled, add dg-options "-Wpedantic" and > > just add dg-warning with c++17_down and c++14_down what should be > > warned on the 3 lines (with .-1 for c++14_down)? > > > > Or if you want add some further testcases that will test how > > c++17 etc. will dg-error on those with -pedantic-errors etc. > > Sure, I've made it { target c++11 } and dropped the third test: > > Bootstrapped/regtested on x86_64-linux, ok for trunk? Just another small comment - given the usual high number of C++ regressions delaying the release is Stage3 the right time to add new language features? > 2018-11-19 Marek Polacek <polacek@redhat.com> > > Implement P1094R2, Nested inline namespaces. > * parser.c (cp_parser_namespace_definition): Parse the optional inline > keyword in a nested-namespace-definition. Adjust push_namespace call. > Formatting fix. > > * g++.dg/cpp2a/nested-inline-ns1.C: New test. > * g++.dg/cpp2a/nested-inline-ns2.C: New test. > > diff --git gcc/cp/parser.c gcc/cp/parser.c > index 292cce15676..f39e9d753d2 100644 > --- gcc/cp/parser.c > +++ gcc/cp/parser.c > @@ -18872,6 +18872,7 @@ cp_parser_namespace_definition (cp_parser* parser) > cp_ensure_no_oacc_routine (parser); > > bool is_inline = cp_lexer_next_token_is_keyword (parser->lexer, RID_INLINE); > + const bool topmost_inline_p = is_inline; > > if (is_inline) > { > @@ -18890,6 +18891,17 @@ cp_parser_namespace_definition (cp_parser* parser) > { > identifier = NULL_TREE; > > + bool nested_inline_p = cp_lexer_next_token_is_keyword (parser->lexer, > + RID_INLINE); > + if (nested_inline_p && nested_definition_count != 0) > + { > + if (cxx_dialect < cxx2a) > + pedwarn (cp_lexer_peek_token (parser->lexer)->location, > + OPT_Wpedantic, "nested inline namespace definitions only " > + "available with -std=c++2a or -std=gnu++2a"); > + cp_lexer_consume_token (parser->lexer); > + } > + > if (cp_lexer_next_token_is (parser->lexer, CPP_NAME)) > { > identifier = cp_parser_identifier (parser); > @@ -18904,7 +18916,12 @@ cp_parser_namespace_definition (cp_parser* parser) > } > > if (cp_lexer_next_token_is_not (parser->lexer, CPP_SCOPE)) > - break; > + { > + /* Don't forget that the innermost namespace might have been > + marked as inline. */ > + is_inline |= nested_inline_p; > + break; > + } > > if (!nested_definition_count && cxx_dialect < cxx17) > pedwarn (input_location, OPT_Wpedantic, > @@ -18913,7 +18930,9 @@ cp_parser_namespace_definition (cp_parser* parser) > > /* Nested namespace names can create new namespaces (unlike > other qualified-ids). */ > - if (int count = identifier ? push_namespace (identifier) : 0) > + if (int count = (identifier > + ? push_namespace (identifier, nested_inline_p) > + : 0)) > nested_definition_count += count; > else > cp_parser_error (parser, "nested namespace name required"); > @@ -18926,7 +18945,7 @@ cp_parser_namespace_definition (cp_parser* parser) > if (nested_definition_count && attribs) > error_at (token->location, > "a nested namespace definition cannot have attributes"); > - if (nested_definition_count && is_inline) > + if (nested_definition_count && topmost_inline_p) > error_at (token->location, > "a nested namespace definition cannot be inline"); > > @@ -18935,7 +18954,7 @@ cp_parser_namespace_definition (cp_parser* parser) > > bool has_visibility = handle_namespace_attrs (current_namespace, attribs); > > - warning (OPT_Wnamespaces, "namespace %qD entered", current_namespace); > + warning (OPT_Wnamespaces, "namespace %qD entered", current_namespace); > > /* Look for the `{' to validate starting the namespace. */ > matching_braces braces; > diff --git gcc/testsuite/g++.dg/cpp2a/nested-inline-ns1.C gcc/testsuite/g++.dg/cpp2a/nested-inline-ns1.C > new file mode 100644 > index 00000000000..8c9573ea5db > --- /dev/null > +++ gcc/testsuite/g++.dg/cpp2a/nested-inline-ns1.C > @@ -0,0 +1,29 @@ > +// P1094R2 > +// { dg-do compile { target c++11 } } > +// { dg-options "-Wpedantic" } > + > +namespace A::inline B::C { // { dg-warning "nested inline namespace definitions only" "" { target c++17_down } } > +// { dg-warning "nested namespace definitions only available" "" { target c++14_down } .-1 } > + int i; > +} > + > +namespace D::E::inline F { // { dg-warning "nested inline namespace definitions only" "" { target c++17_down } } > +// { dg-warning "nested namespace definitions only available" "" { target c++14_down } .-1 } > + int j; > +} > + > +inline namespace X { > + int x; > +} > + > +// Make sure the namespaces are marked inline. > +void > +g () > +{ > + A::B::C::i++; > + A::C::i++; > + D::E::j++; > + D::E::F::j++; > + X::x++; > + x++; > +} > diff --git gcc/testsuite/g++.dg/cpp2a/nested-inline-ns2.C gcc/testsuite/g++.dg/cpp2a/nested-inline-ns2.C > new file mode 100644 > index 00000000000..9b5f2cab47b > --- /dev/null > +++ gcc/testsuite/g++.dg/cpp2a/nested-inline-ns2.C > @@ -0,0 +1,26 @@ > +// P1094R2 > +// { dg-do compile { target c++2a } } > + > +inline namespace A::B { // { dg-error "a nested namespace definition cannot be inline" } > + int i; > +} > + > +namespace inline C::D { // { dg-error "expected|does not name a type" } > + int i; > +} > + > +namespace E::F inline { // { dg-error "expected" } > + int i; > +} > + > +namespace inline G { // { dg-error "expected|does not name a type" } > + int i; > +} > + > +inline namespace inline H { // { dg-error "expected|does not name a type" } > + int i; > +} > + > +inline namespace inline I::J { // { dg-error "expected|does not name a type" } > + int i; > +}
On Tue, Nov 20, 2018 at 10:25:01AM +0100, Richard Biener wrote: > > Bootstrapped/regtested on x86_64-linux, ok for trunk? > > Just another small comment - given the usual high number of > C++ regressions delaying the release is Stage3 the right time > to add new language features? I'd say this is small enough and worth an exception, it is just useful syntactic sugar, and couldn't be submitted (much) earlier as it has been voted in during the week when stage1 closed. Jakub
On Tue, Nov 20, 2018 at 10:36:32AM +0100, Jakub Jelinek wrote: > On Tue, Nov 20, 2018 at 10:25:01AM +0100, Richard Biener wrote: > > > Bootstrapped/regtested on x86_64-linux, ok for trunk? > > > > Just another small comment - given the usual high number of > > C++ regressions delaying the release is Stage3 the right time > > to add new language features? > > I'd say this is small enough and worth an exception, it is just useful syntactic > sugar, and couldn't be submitted (much) earlier as it has been voted in > during the week when stage1 closed. Yeah, this one is very low risk and I think it would be nice to have it in for GCC 9 to make the C++20 support better. FWIW, I don't plan on working on other C++20 features for this stage; I'll be addressing C++ bugs now. Marek
On 11/19/18 5:12 PM, Marek Polacek wrote: > On Mon, Nov 19, 2018 at 10:33:17PM +0100, Jakub Jelinek wrote: >> On Mon, Nov 19, 2018 at 04:21:19PM -0500, Marek Polacek wrote: >>> 2018-11-19 Marek Polacek <polacek@redhat.com> >>> >>> Implement P1094R2, Nested inline namespaces. >>> * g++.dg/cpp2a/nested-inline-ns1.C: New test. >>> * g++.dg/cpp2a/nested-inline-ns2.C: New test. >>> * g++.dg/cpp2a/nested-inline-ns3.C: New test. >> >> Just a small testsuite comment. >> >>> --- /dev/null >>> +++ gcc/testsuite/g++.dg/cpp2a/nested-inline-ns1.C >>> @@ -0,0 +1,26 @@ >>> +// P1094R2 >>> +// { dg-do compile { target c++2a } } >> >> Especially because 2a testing isn't included by default, but also >> to make sure it works right even with -std=c++17, wouldn't it be better to >> drop the nested-inline-ns3.C test, make this test c++17 or >> even better always enabled, add dg-options "-Wpedantic" and >> just add dg-warning with c++17_down and c++14_down what should be >> warned on the 3 lines (with .-1 for c++14_down)? >> >> Or if you want add some further testcases that will test how >> c++17 etc. will dg-error on those with -pedantic-errors etc. > > Sure, I've made it { target c++11 } and dropped the third test: > > Bootstrapped/regtested on x86_64-linux, ok for trunk? > > 2018-11-19 Marek Polacek <polacek@redhat.com> > > Implement P1094R2, Nested inline namespaces. > * parser.c (cp_parser_namespace_definition): Parse the optional inline > keyword in a nested-namespace-definition. Adjust push_namespace call. > Formatting fix. > > * g++.dg/cpp2a/nested-inline-ns1.C: New test. > * g++.dg/cpp2a/nested-inline-ns2.C: New test. > > diff --git gcc/cp/parser.c gcc/cp/parser.c > index 292cce15676..f39e9d753d2 100644 > --- gcc/cp/parser.c > +++ gcc/cp/parser.c > @@ -18872,6 +18872,7 @@ cp_parser_namespace_definition (cp_parser* parser) > cp_ensure_no_oacc_routine (parser); > > bool is_inline = cp_lexer_next_token_is_keyword (parser->lexer, RID_INLINE); > + const bool topmost_inline_p = is_inline; > > if (is_inline) > { > @@ -18890,6 +18891,17 @@ cp_parser_namespace_definition (cp_parser* parser) > { > identifier = NULL_TREE; > > + bool nested_inline_p = cp_lexer_next_token_is_keyword (parser->lexer, > + RID_INLINE); > + if (nested_inline_p && nested_definition_count != 0) > + { > + if (cxx_dialect < cxx2a) > + pedwarn (cp_lexer_peek_token (parser->lexer)->location, > + OPT_Wpedantic, "nested inline namespace definitions only " > + "available with -std=c++2a or -std=gnu++2a"); > + cp_lexer_consume_token (parser->lexer); > + } This looks like we won't get any diagnostic in lower conformance modes if there are multiple namespace scopes before the inline keyword. > if (cp_lexer_next_token_is (parser->lexer, CPP_NAME)) > { > identifier = cp_parser_identifier (parser); > @@ -18904,7 +18916,12 @@ cp_parser_namespace_definition (cp_parser* parser) > } > > if (cp_lexer_next_token_is_not (parser->lexer, CPP_SCOPE)) > - break; > + { > + /* Don't forget that the innermost namespace might have been > + marked as inline. */ > + is_inline |= nested_inline_p; This looks wrong: an inline namespace does not make its nested namespaces inline as well. Jason
On Tue, Nov 20, 2018 at 04:59:46PM -0500, Jason Merrill wrote: > On 11/19/18 5:12 PM, Marek Polacek wrote: > > On Mon, Nov 19, 2018 at 10:33:17PM +0100, Jakub Jelinek wrote: > > > On Mon, Nov 19, 2018 at 04:21:19PM -0500, Marek Polacek wrote: > > > > 2018-11-19 Marek Polacek <polacek@redhat.com> > > > > > > > > Implement P1094R2, Nested inline namespaces. > > > > * g++.dg/cpp2a/nested-inline-ns1.C: New test. > > > > * g++.dg/cpp2a/nested-inline-ns2.C: New test. > > > > * g++.dg/cpp2a/nested-inline-ns3.C: New test. > > > > > > Just a small testsuite comment. > > > > > > > --- /dev/null > > > > +++ gcc/testsuite/g++.dg/cpp2a/nested-inline-ns1.C > > > > @@ -0,0 +1,26 @@ > > > > +// P1094R2 > > > > +// { dg-do compile { target c++2a } } > > > > > > Especially because 2a testing isn't included by default, but also > > > to make sure it works right even with -std=c++17, wouldn't it be better to > > > drop the nested-inline-ns3.C test, make this test c++17 or > > > even better always enabled, add dg-options "-Wpedantic" and > > > just add dg-warning with c++17_down and c++14_down what should be > > > warned on the 3 lines (with .-1 for c++14_down)? > > > > > > Or if you want add some further testcases that will test how > > > c++17 etc. will dg-error on those with -pedantic-errors etc. > > > > Sure, I've made it { target c++11 } and dropped the third test: > > > > Bootstrapped/regtested on x86_64-linux, ok for trunk? > > > > 2018-11-19 Marek Polacek <polacek@redhat.com> > > > > Implement P1094R2, Nested inline namespaces. > > * parser.c (cp_parser_namespace_definition): Parse the optional inline > > keyword in a nested-namespace-definition. Adjust push_namespace call. > > Formatting fix. > > > > * g++.dg/cpp2a/nested-inline-ns1.C: New test. > > * g++.dg/cpp2a/nested-inline-ns2.C: New test. > > > > diff --git gcc/cp/parser.c gcc/cp/parser.c > > index 292cce15676..f39e9d753d2 100644 > > --- gcc/cp/parser.c > > +++ gcc/cp/parser.c > > @@ -18872,6 +18872,7 @@ cp_parser_namespace_definition (cp_parser* parser) > > cp_ensure_no_oacc_routine (parser); > > bool is_inline = cp_lexer_next_token_is_keyword (parser->lexer, RID_INLINE); > > + const bool topmost_inline_p = is_inline; > > if (is_inline) > > { > > @@ -18890,6 +18891,17 @@ cp_parser_namespace_definition (cp_parser* parser) > > { > > identifier = NULL_TREE; > > + bool nested_inline_p = cp_lexer_next_token_is_keyword (parser->lexer, > > + RID_INLINE); > > + if (nested_inline_p && nested_definition_count != 0) > > + { > > + if (cxx_dialect < cxx2a) > > + pedwarn (cp_lexer_peek_token (parser->lexer)->location, > > + OPT_Wpedantic, "nested inline namespace definitions only " > > + "available with -std=c++2a or -std=gnu++2a"); > > + cp_lexer_consume_token (parser->lexer); > > + } > > This looks like we won't get any diagnostic in lower conformance modes if > there are multiple namespace scopes before the inline keyword. If you mean something like namespace A::B:C::inline D { } then we do get a diagnostic. nested-inline-ns1.C tests that. Or do you mean something else? > > if (cp_lexer_next_token_is (parser->lexer, CPP_NAME)) > > { > > identifier = cp_parser_identifier (parser); > > @@ -18904,7 +18916,12 @@ cp_parser_namespace_definition (cp_parser* parser) > > } > > if (cp_lexer_next_token_is_not (parser->lexer, CPP_SCOPE)) > > - break; > > + { > > + /* Don't forget that the innermost namespace might have been > > + marked as inline. */ > > + is_inline |= nested_inline_p; > > This looks wrong: an inline namespace does not make its nested namespaces > inline as well. A nested namespace definition cannot be inline. This is supposed to handle cases such as namespace A::B::inline C { ... } because after 'C' we don't see :: so it breaks and we call push_namespace outside the for loop. So I still don't see a bug; do you have a test that I got wrong? Marek
On 11/21/18 8:46 PM, Marek Polacek wrote: > On Tue, Nov 20, 2018 at 04:59:46PM -0500, Jason Merrill wrote: >> On 11/19/18 5:12 PM, Marek Polacek wrote: >>> On Mon, Nov 19, 2018 at 10:33:17PM +0100, Jakub Jelinek wrote: >>>> On Mon, Nov 19, 2018 at 04:21:19PM -0500, Marek Polacek wrote: >>>>> 2018-11-19 Marek Polacek <polacek@redhat.com> >>>>> >>>>> Implement P1094R2, Nested inline namespaces. >>>>> * g++.dg/cpp2a/nested-inline-ns1.C: New test. >>>>> * g++.dg/cpp2a/nested-inline-ns2.C: New test. >>>>> * g++.dg/cpp2a/nested-inline-ns3.C: New test. >>>> >>>> Just a small testsuite comment. >>>> >>>>> --- /dev/null >>>>> +++ gcc/testsuite/g++.dg/cpp2a/nested-inline-ns1.C >>>>> @@ -0,0 +1,26 @@ >>>>> +// P1094R2 >>>>> +// { dg-do compile { target c++2a } } >>>> >>>> Especially because 2a testing isn't included by default, but also >>>> to make sure it works right even with -std=c++17, wouldn't it be better to >>>> drop the nested-inline-ns3.C test, make this test c++17 or >>>> even better always enabled, add dg-options "-Wpedantic" and >>>> just add dg-warning with c++17_down and c++14_down what should be >>>> warned on the 3 lines (with .-1 for c++14_down)? >>>> >>>> Or if you want add some further testcases that will test how >>>> c++17 etc. will dg-error on those with -pedantic-errors etc. >>> >>> Sure, I've made it { target c++11 } and dropped the third test: >>> >>> Bootstrapped/regtested on x86_64-linux, ok for trunk? >>> >>> 2018-11-19 Marek Polacek <polacek@redhat.com> >>> >>> Implement P1094R2, Nested inline namespaces. >>> * parser.c (cp_parser_namespace_definition): Parse the optional inline >>> keyword in a nested-namespace-definition. Adjust push_namespace call. >>> Formatting fix. >>> >>> * g++.dg/cpp2a/nested-inline-ns1.C: New test. >>> * g++.dg/cpp2a/nested-inline-ns2.C: New test. >>> >>> diff --git gcc/cp/parser.c gcc/cp/parser.c >>> index 292cce15676..f39e9d753d2 100644 >>> --- gcc/cp/parser.c >>> +++ gcc/cp/parser.c >>> @@ -18872,6 +18872,7 @@ cp_parser_namespace_definition (cp_parser* parser) >>> cp_ensure_no_oacc_routine (parser); >>> bool is_inline = cp_lexer_next_token_is_keyword (parser->lexer, RID_INLINE); >>> + const bool topmost_inline_p = is_inline; >>> if (is_inline) >>> { >>> @@ -18890,6 +18891,17 @@ cp_parser_namespace_definition (cp_parser* parser) >>> { >>> identifier = NULL_TREE; >>> + bool nested_inline_p = cp_lexer_next_token_is_keyword (parser->lexer, >>> + RID_INLINE); >>> + if (nested_inline_p && nested_definition_count != 0) >>> + { >>> + if (cxx_dialect < cxx2a) >>> + pedwarn (cp_lexer_peek_token (parser->lexer)->location, >>> + OPT_Wpedantic, "nested inline namespace definitions only " >>> + "available with -std=c++2a or -std=gnu++2a"); >>> + cp_lexer_consume_token (parser->lexer); >>> + } >> >> This looks like we won't get any diagnostic in lower conformance modes if >> there are multiple namespace scopes before the inline keyword. > > If you mean something like > namespace A::B:C::inline D { } > then we do get a diagnostic. nested-inline-ns1.C tests that. Or do you > mean something else? No, this is fine then. >>> if (cp_lexer_next_token_is (parser->lexer, CPP_NAME)) >>> { >>> identifier = cp_parser_identifier (parser); >>> @@ -18904,7 +18916,12 @@ cp_parser_namespace_definition (cp_parser* parser) >>> } >>> if (cp_lexer_next_token_is_not (parser->lexer, CPP_SCOPE)) >>> - break; >>> + { >>> + /* Don't forget that the innermost namespace might have been >>> + marked as inline. */ >>> + is_inline |= nested_inline_p; >> >> This looks wrong: an inline namespace does not make its nested namespaces >> inline as well. > A nested namespace definition cannot be inline. This is supposed to handle > cases such as > namespace A::B::inline C { ... } > because after 'C' we don't see :: so it breaks and we call push_namespace > outside the for loop. Ah, yes, I see. I was confused by the use of '|='; what is that for? Why not use '='? For that matter, why not modify is_inline in the loop instead of a new nested_inline_p variable? Jason
On Tue, Nov 27, 2018 at 04:01:46PM -0500, Jason Merrill wrote: > > > > if (cp_lexer_next_token_is (parser->lexer, CPP_NAME)) > > > > { > > > > identifier = cp_parser_identifier (parser); > > > > @@ -18904,7 +18916,12 @@ cp_parser_namespace_definition (cp_parser* parser) > > > > } > > > > if (cp_lexer_next_token_is_not (parser->lexer, CPP_SCOPE)) > > > > - break; > > > > + { > > > > + /* Don't forget that the innermost namespace might have been > > > > + marked as inline. */ > > > > + is_inline |= nested_inline_p; > > > > > > This looks wrong: an inline namespace does not make its nested namespaces > > > inline as well. > > > A nested namespace definition cannot be inline. This is supposed to handle > > cases such as > > namespace A::B::inline C { ... } > > because after 'C' we don't see :: so it breaks and we call push_namespace > > outside the for loop. > > Ah, yes, I see. I was confused by the use of '|='; what is that for? Why > not use '='? For that matter, why not modify is_inline in the loop instead > of a new nested_inline_p variable? |= is there to handle correctly this case: inline namespace N { ... } where we have to make sure that the call to push_namespace outside the for (;;) is with is_inline=true. Using '=' would overwrite is_inline to false, because there are no nested inlines. For the same reason I needed a second bool: to not lose the information about the topmost inline. But since the second push_namespace call also needs to handle the innermost namespace, it can't use topmost_inline_p. Marek
On Wed, Nov 28, 2018 at 11:57 AM Marek Polacek <polacek@redhat.com> wrote: > > On Tue, Nov 27, 2018 at 04:01:46PM -0500, Jason Merrill wrote: > > > > > if (cp_lexer_next_token_is (parser->lexer, CPP_NAME)) > > > > > { > > > > > identifier = cp_parser_identifier (parser); > > > > > @@ -18904,7 +18916,12 @@ cp_parser_namespace_definition (cp_parser* parser) > > > > > } > > > > > if (cp_lexer_next_token_is_not (parser->lexer, CPP_SCOPE)) > > > > > - break; > > > > > + { > > > > > + /* Don't forget that the innermost namespace might have been > > > > > + marked as inline. */ > > > > > + is_inline |= nested_inline_p; > > > > > > > > This looks wrong: an inline namespace does not make its nested namespaces > > > > inline as well. > > > > > A nested namespace definition cannot be inline. This is supposed to handle > > > cases such as > > > namespace A::B::inline C { ... } > > > because after 'C' we don't see :: so it breaks and we call push_namespace > > > outside the for loop. > > > > Ah, yes, I see. I was confused by the use of '|='; what is that for? Why > > not use '='? For that matter, why not modify is_inline in the loop instead > > of a new nested_inline_p variable? > > |= is there to handle correctly this case: > inline namespace N { ... } > where we have to make sure that the call to push_namespace outside the for (;;) > is with is_inline=true. Using '=' would overwrite is_inline to false, because > there are no nested inlines. For the same reason I needed a second bool: to > not lose the information about the topmost inline. But since the second > push_namespace call also needs to handle the innermost namespace, it can't use > topmost_inline_p. Aha. Please mention that in the comment. OK with that change. Jason
diff --git gcc/cp/parser.c gcc/cp/parser.c index 88fc426102b..8b8304acca7 100644 --- gcc/cp/parser.c +++ gcc/cp/parser.c @@ -18864,6 +18864,7 @@ cp_parser_namespace_definition (cp_parser* parser) cp_ensure_no_oacc_routine (parser); bool is_inline = cp_lexer_next_token_is_keyword (parser->lexer, RID_INLINE); + const bool topmost_inline_p = is_inline; if (is_inline) { @@ -18882,6 +18883,17 @@ cp_parser_namespace_definition (cp_parser* parser) { identifier = NULL_TREE; + bool nested_inline_p = cp_lexer_next_token_is_keyword (parser->lexer, + RID_INLINE); + if (nested_inline_p && nested_definition_count != 0) + { + if (cxx_dialect < cxx2a) + pedwarn (cp_lexer_peek_token (parser->lexer)->location, + OPT_Wpedantic, "nested inline namespace definitions only " + "available with -std=c++2a or -std=gnu++2a"); + cp_lexer_consume_token (parser->lexer); + } + if (cp_lexer_next_token_is (parser->lexer, CPP_NAME)) { identifier = cp_parser_identifier (parser); @@ -18896,7 +18908,12 @@ cp_parser_namespace_definition (cp_parser* parser) } if (cp_lexer_next_token_is_not (parser->lexer, CPP_SCOPE)) - break; + { + /* Don't forget that the innermost namespace might have been + marked as inline. */ + is_inline |= nested_inline_p; + break; + } if (!nested_definition_count && cxx_dialect < cxx17) pedwarn (input_location, OPT_Wpedantic, @@ -18905,7 +18922,9 @@ cp_parser_namespace_definition (cp_parser* parser) /* Nested namespace names can create new namespaces (unlike other qualified-ids). */ - if (int count = identifier ? push_namespace (identifier) : 0) + if (int count = (identifier + ? push_namespace (identifier, nested_inline_p) + : 0)) nested_definition_count += count; else cp_parser_error (parser, "nested namespace name required"); @@ -18918,7 +18937,7 @@ cp_parser_namespace_definition (cp_parser* parser) if (nested_definition_count && attribs) error_at (token->location, "a nested namespace definition cannot have attributes"); - if (nested_definition_count && is_inline) + if (nested_definition_count && topmost_inline_p) error_at (token->location, "a nested namespace definition cannot be inline"); @@ -18927,7 +18946,7 @@ cp_parser_namespace_definition (cp_parser* parser) bool has_visibility = handle_namespace_attrs (current_namespace, attribs); - warning (OPT_Wnamespaces, "namespace %qD entered", current_namespace); + warning (OPT_Wnamespaces, "namespace %qD entered", current_namespace); /* Look for the `{' to validate starting the namespace. */ matching_braces braces; diff --git gcc/testsuite/g++.dg/cpp2a/nested-inline-ns1.C gcc/testsuite/g++.dg/cpp2a/nested-inline-ns1.C new file mode 100644 index 00000000000..95b4d3378d1 --- /dev/null +++ gcc/testsuite/g++.dg/cpp2a/nested-inline-ns1.C @@ -0,0 +1,26 @@ +// P1094R2 +// { dg-do compile { target c++2a } } + +namespace A::inline B::C { + int i; +} + +namespace D::E::inline F { + int j; +} + +inline namespace X { + int x; +} + +// Make sure the namespaces are marked inline. +void +g () +{ + A::B::C::i++; + A::C::i++; + D::E::j++; + D::E::F::j++; + X::x++; + x++; +} diff --git gcc/testsuite/g++.dg/cpp2a/nested-inline-ns2.C gcc/testsuite/g++.dg/cpp2a/nested-inline-ns2.C new file mode 100644 index 00000000000..9b5f2cab47b --- /dev/null +++ gcc/testsuite/g++.dg/cpp2a/nested-inline-ns2.C @@ -0,0 +1,26 @@ +// P1094R2 +// { dg-do compile { target c++2a } } + +inline namespace A::B { // { dg-error "a nested namespace definition cannot be inline" } + int i; +} + +namespace inline C::D { // { dg-error "expected|does not name a type" } + int i; +} + +namespace E::F inline { // { dg-error "expected" } + int i; +} + +namespace inline G { // { dg-error "expected|does not name a type" } + int i; +} + +inline namespace inline H { // { dg-error "expected|does not name a type" } + int i; +} + +inline namespace inline I::J { // { dg-error "expected|does not name a type" } + int i; +} diff --git gcc/testsuite/g++.dg/cpp2a/nested-inline-ns3.C gcc/testsuite/g++.dg/cpp2a/nested-inline-ns3.C new file mode 100644 index 00000000000..8b81c1383db --- /dev/null +++ gcc/testsuite/g++.dg/cpp2a/nested-inline-ns3.C @@ -0,0 +1,11 @@ +// P1094R2 +// { dg-do compile { target c++17 } } +// { dg-options "-Wpedantic" } + +namespace A::inline B::C { // { dg-warning "nested inline namespace definitions only" "" { target c++17_down } } + int i; +} + +namespace D::E::inline F { // { dg-warning "nested inline namespace definitions only" "" { target c++17_down } } + int i; +}
This patch implements another C++20 feature, nested inline namespaces. This was fairly simple, one just has to be careful not to blithely consume the inline keyword, making a non-valid program valid. Another minor gotcha was to handle the innermost 'inline' correctly. Note that inline namespace A::B { ... } continues to be invalid. Bootstrapped/regtested on x86_64-linux, ok for trunk? 2018-11-19 Marek Polacek <polacek@redhat.com> Implement P1094R2, Nested inline namespaces. * parser.c (cp_parser_namespace_definition): Parse the optional inline keyword in a nested-namespace-definition. Adjust push_namespace call. Formatting fix. * g++.dg/cpp2a/nested-inline-ns1.C: New test. * g++.dg/cpp2a/nested-inline-ns2.C: New test. * g++.dg/cpp2a/nested-inline-ns3.C: New test.