On Sat, May 21, 2011 at 9:15 AM, Douglas Gregor <[email protected]> wrote:
>
> On May 19, 2011, at 4:14 PM, Richard Trieu wrote:
>
>
>
> On Sat, May 7, 2011 at 9:59 AM, Douglas Gregor <[email protected]> wrote:
>
>>
>> On May 6, 2011, at 6:57 PM, Richard Trieu wrote:
>>
>> Moved fix-its from the notes to the error message.
>>
>> On Fri, May 6, 2011 at 2:20 PM, Richard Trieu <[email protected]> wrote:
>>
>>> When improperly using "::" to nest namespaces, give a fix-it to correct
>>> it. Parsing will also recover as if they were properly nested.
>>>
>>> namespace foo::bar {
>>> }
>>>
>>> Will result in a fix-it at "::" to change to " { namespace " and an
>>> additional right bracket at the end.
>>>
>>> Patch attached and available through Code Review:
>>> http://codereview.appspot.com/4452052/
>>>
>>
>> + // Unless we have "namepsace foo::bar {", just complain about a lack
>> of '{'.
>>
>> Typo "namepsace".
>>
>> + if (!Tok.is(tok::coloncolon) || !NextToken().is(tok::identifier)) {
>> + Diag(Tok, Ident ? diag::err_expected_lbrace :
>> + diag::err_expected_ident_lbrace);
>> + return 0;
>> + }
>>
>> + // Otherwise, recover parsing as if the user had typed
>> + // "namespace foo { namespace bar {".
>> + TentativeParsingAction TPA(*this);
>> + SkipUntil(tok::l_brace, /*StopAtSemi*/false);
>> + SkipUntil(tok::r_brace, /*StopAtSemi*/false, /*DontConsume*/true);
>> + Token rBraceToken = Tok;
>> + rBraceToken.setKind(tok::r_brace);
>> + PP.EnterToken(rBraceToken);
>> + TPA.Revert();
>>
>> IMO, this isn't robust enough for a Fix-It, because it isn't actually
>> checking for the presence of the '{' or the '}'. For example, if the
>> "namespace foo::bar" was at the end of the translation unit, it looks like
>> we could end up trying to change the end-of-file token to a right-brace.
>>
>> I think it's fine to use tentative parsing here, but it should only be
>> used to identify where the '{' and '}' tokens are so we know that this could
>> be a well-formed namespace definition. Once we've figured that out, I'd
>> prefer that the code not try to change the token stream. Rather, I think it
>> would be better just to parse the
>>
>> namespace foo::bar {
>> and call the appropriate semantic actions to start the foo namespace,
>> then the bar namespace, and keep track of the results of each of those
>> calls. Then, when we see the closing '}', call the appropriate semantic
>> actions to complete those namespace definitions.
>>
>> Also, this code is assuming that we're parsing something like
>>
>> namespace foo::bar {
>> }
>>
>> but not
>>
>> namespace foo::bar::wibble {
>> }
>>
>> Why not generalize it to an arbitrary-length identifier-and-::
>> definitions? If this were a language feature, we'd want it to support an
>> arbitrary depth.
>>
>> - Doug
>>
>
> Changed how the error recovery works. Tentative parsing is still used to
> locate the closing '}', but the token stream is no longer altered. Instead
> of an error for every nested namespace, only produce one error overall with
> the fix-its. Error message will also trigger, but without the fix-its, if
> the namespace is not properly formed. Arbitrary-length nested-namespaces
> are now handled.
>
>
> Great! Only two comments, then I think this patch can go in:
>
> + std::string NamespaceFix = "";
> + for (std::vector<IdentifierInfo*>::iterator I = ExtraIdent.begin(),
> + E = ExtraIdent.end(); I != E; ++I) {
> + NamespaceFix += " { namespace ";
> + NamespaceFix += (*I)->getName();
> + }
> + std::string RBraces(ExtraIdent.size(), '}');
>
> It's just stylistic, but I'd prefer that that RBraces have spaces between
> each '}'.
>
> + // Parse improperly nested namespaces.
> + ParseScope NamespaceScope(this, Scope::DeclScope);
> + Decl *NamespcDecl =
> + Actions.ActOnStartNamespaceDef(getCurScope(), InlineLoc,
> + NamespaceLoc[index], IdentLoc[index],
> + Ident[index], LBrace, attrs.getList());
>
> I think that InlineLoc should only be passed through for the innermost
> namespace, e.g., given
>
> inline namespace foo::bar {
> }
>
> we should pass SourceLocation() for the location of "inline" for "foo", but
> pass InlineLoc for the location of "inline" for "bar".
>
> Thanks for working on this!
>
> - Doug
>
> Okay, I made the two changes you brought up. All look good?
Index: test/Parser/nested-namespaces-recovery.cpp
===================================================================
--- test/Parser/nested-namespaces-recovery.cpp (revision 0)
+++ test/Parser/nested-namespaces-recovery.cpp (revision 0)
@@ -0,0 +1,24 @@
+// RUN: cp %s %t
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: not %clang_cc1 -x c++ -fixit %t
+// RUN: %clang_cc1 -x c++ %t
+
+namespace foo1::foo2::foo3 { // expected-error {{nested namespace definition must define each namespace separately}}
+ int foo(int x) { return x; }
+}
+
+int foo(int x) {
+ return foo1::foo2::foo3::foo(x);
+}
+
+namespace bar1 {
+ namespace bar2 {
+ namespace bar3 {
+ int bar(int x) { return x; }
+ }
+ }
+}
+
+int bar(int x) {
+ return bar1::bar2::bar3::bar(x);
+}
Index: include/clang/Basic/DiagnosticParseKinds.td
===================================================================
--- include/clang/Basic/DiagnosticParseKinds.td (revision 130254)
+++ include/clang/Basic/DiagnosticParseKinds.td (working copy)
@@ -161,6 +161,8 @@
def err_inline_namespace_alias : Error<"namespace alias cannot be inline">;
def err_namespace_nonnamespace_scope : Error<
"namespaces can only be defined in global or namespace scope">;
+def err_nested_namespaces_with_double_colon : Error<
+ "nested namespace definition must define each namespace separately">;
def err_expected_semi_after_attribute_list : Error<
"expected ';' after attribute list">;
def err_expected_semi_after_static_assert : Error<
Index: include/clang/Parse/Parser.h
===================================================================
--- include/clang/Parse/Parser.h (revision 130254)
+++ include/clang/Parse/Parser.h (working copy)
@@ -1686,6 +1686,12 @@
Decl *ParseNamespace(unsigned Context, SourceLocation &DeclEnd,
SourceLocation InlineLoc = SourceLocation());
+ void ParseInnerNamespace(std::vector<SourceLocation>& IdentLoc,
+ std::vector<IdentifierInfo*>& Ident,
+ std::vector<SourceLocation>& NamespaceLoc,
+ unsigned int index, SourceLocation& InlineLoc,
+ SourceLocation& LBrace, ParsedAttributes& attrs,
+ SourceLocation& RBraceLoc);
Decl *ParseLinkage(ParsingDeclSpec &DS, unsigned Context);
Decl *ParseUsingDirectiveOrDeclaration(unsigned Context,
const ParsedTemplateInfo &TemplateInfo,
Index: lib/Parse/ParseDeclCXX.cpp
===================================================================
--- lib/Parse/ParseDeclCXX.cpp (revision 130254)
+++ lib/Parse/ParseDeclCXX.cpp (working copy)
@@ -60,12 +60,20 @@
SourceLocation IdentLoc;
IdentifierInfo *Ident = 0;
+ std::vector<SourceLocation> ExtraIdentLoc;
+ std::vector<IdentifierInfo*> ExtraIdent;
+ std::vector<SourceLocation> ExtraNamespaceLoc;
Token attrTok;
if (Tok.is(tok::identifier)) {
Ident = Tok.getIdentifierInfo();
IdentLoc = ConsumeToken(); // eat the identifier.
+ while (Tok.is(tok::coloncolon) && NextToken().is(tok::identifier)) {
+ ExtraNamespaceLoc.push_back(ConsumeToken());
+ ExtraIdent.push_back(Tok.getIdentifierInfo());
+ ExtraIdentLoc.push_back(ConsumeToken());
+ }
}
// Read label attributes, if present.
@@ -85,7 +93,12 @@
return ParseNamespaceAlias(NamespaceLoc, IdentLoc, Ident, DeclEnd);
}
+
if (Tok.isNot(tok::l_brace)) {
+ if (!ExtraIdent.empty()) {
+ Diag(ExtraNamespaceLoc[0], diag::err_nested_namespaces_with_double_colon)
+ << SourceRange(ExtraNamespaceLoc.front(), ExtraIdentLoc.back());
+ }
Diag(Tok, Ident ? diag::err_expected_lbrace :
diag::err_expected_ident_lbrace);
return 0;
@@ -96,11 +109,44 @@
if (getCurScope()->isClassScope() || getCurScope()->isTemplateParamScope() ||
getCurScope()->isInObjcMethodScope() || getCurScope()->getBlockParent() ||
getCurScope()->getFnParent()) {
+ if (!ExtraIdent.empty()) {
+ Diag(ExtraNamespaceLoc[0], diag::err_nested_namespaces_with_double_colon)
+ << SourceRange(ExtraNamespaceLoc.front(), ExtraIdentLoc.back());
+ }
Diag(LBrace, diag::err_namespace_nonnamespace_scope);
SkipUntil(tok::r_brace, false);
return 0;
}
+ if (!ExtraIdent.empty()) {
+ TentativeParsingAction TPA(*this);
+ SkipUntil(tok::r_brace, /*StopAtSemi*/false, /*DontConsume*/true);
+ Token rBraceToken = Tok;
+ TPA.Revert();
+
+ if (!rBraceToken.is(tok::r_brace)) {
+ Diag(ExtraNamespaceLoc[0], diag::err_nested_namespaces_with_double_colon)
+ << SourceRange(ExtraNamespaceLoc.front(), ExtraIdentLoc.back());
+ } else {
+
+ std::string NamespaceFix = "";
+ for (std::vector<IdentifierInfo*>::iterator I = ExtraIdent.begin(),
+ E = ExtraIdent.end(); I != E; ++I) {
+ NamespaceFix += " { namespace ";
+ NamespaceFix += (*I)->getName();
+ }
+ std::string RBraces;
+ for (int i = 0; i < ExtraIdent.size(); ++i) {
+ RBraces += "} ";
+ }
+ Diag(ExtraNamespaceLoc[0], diag::err_nested_namespaces_with_double_colon)
+ << FixItHint::CreateReplacement(SourceRange(ExtraNamespaceLoc.front(),
+ ExtraIdentLoc.back()),
+ NamespaceFix)
+ << FixItHint::CreateInsertion(rBraceToken.getLocation(), RBraces);
+ }
+ }
+
// If we're still good, complain about inline namespaces in non-C++0x now.
if (!getLang().CPlusPlus0x && InlineLoc.isValid())
Diag(InlineLoc, diag::ext_inline_namespace);
@@ -115,23 +161,56 @@
PrettyDeclStackTraceEntry CrashInfo(Actions, NamespcDecl, NamespaceLoc,
"parsing namespace");
- while (Tok.isNot(tok::r_brace) && Tok.isNot(tok::eof)) {
- ParsedAttributesWithRange attrs(AttrFactory);
- MaybeParseCXX0XAttributes(attrs);
- MaybeParseMicrosoftAttributes(attrs);
- ParseExternalDeclaration(attrs);
- }
+ SourceLocation RBraceLoc;
+ // Parse the contents of the namespace. This includes parsing recovery on
+ // any improperly nested namespaces.
+ ParseInnerNamespace(ExtraIdentLoc, ExtraIdent, ExtraNamespaceLoc, 0,
+ InlineLoc, LBrace, attrs, RBraceLoc);
// Leave the namespace scope.
NamespaceScope.Exit();
- SourceLocation RBraceLoc = MatchRHSPunctuation(tok::r_brace, LBrace);
Actions.ActOnFinishNamespaceDef(NamespcDecl, RBraceLoc);
DeclEnd = RBraceLoc;
return NamespcDecl;
}
+/// ParseInnerNamespace - Parse the contents of a namespace.
+void Parser::ParseInnerNamespace(std::vector<SourceLocation>& IdentLoc,
+ std::vector<IdentifierInfo*>& Ident,
+ std::vector<SourceLocation>& NamespaceLoc,
+ unsigned int index, SourceLocation& InlineLoc,
+ SourceLocation& LBrace,
+ ParsedAttributes& attrs,
+ SourceLocation& RBraceLoc) {
+ if (index == Ident.size()) {
+ while (Tok.isNot(tok::r_brace) && Tok.isNot(tok::eof)) {
+ ParsedAttributesWithRange attrs(AttrFactory);
+ MaybeParseCXX0XAttributes(attrs);
+ MaybeParseMicrosoftAttributes(attrs);
+ ParseExternalDeclaration(attrs);
+ }
+ RBraceLoc = MatchRHSPunctuation(tok::r_brace, LBrace);
+
+ return;
+ }
+
+ // Parse improperly nested namespaces.
+ ParseScope NamespaceScope(this, Scope::DeclScope);
+ Decl *NamespcDecl =
+ Actions.ActOnStartNamespaceDef(getCurScope(), SourceLocation(),
+ NamespaceLoc[index], IdentLoc[index],
+ Ident[index], LBrace, attrs.getList());
+
+ ParseInnerNamespace(IdentLoc, Ident, NamespaceLoc, ++index, InlineLoc,
+ LBrace, attrs, RBraceLoc);
+
+ NamespaceScope.Exit();
+
+ Actions.ActOnFinishNamespaceDef(NamespcDecl, RBraceLoc);
+}
+
/// ParseNamespaceAlias - Parse the part after the '=' in a namespace
/// alias definition.
///
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
