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 <pola...@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 <pola...@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
