[cfe-commits] [patch] PR10127/N3031 supporting decltype in nested-namespace-specifiers

[Douglas Gregor]

On Nov 7, 2011, at 7:44 AM, David Blaikie wrote:

> Now with better error recovery (utilizing CXXScopeSpec's
> setInvalid(SourceRange) as is done elsewhere) & support for
> non-crashing when used in member ref expressions like
> a->decltype(foo)::bar() (including test cases for same) based on some
> test cases suggested by Chandler Carruth.

Some comments follow…

Index: include/clang/Basic/TokenKinds.def
===================================================================
--- include/clang/Basic/TokenKinds.def	(revision 143902)
+++ include/clang/Basic/TokenKinds.def	(working copy)
@@ -559,6 +559,8 @@
                          // function template specialization (not a type),
                          // e.g., "std::swap<int>"
 ANNOTATION(primary_expr) // annotation for a primary expression
+ANNOTATION(decltype)     // annotation for a decltype expression,
+                         // e.g., "decltype(foo.bar())"
 
 // Annotation for #pragma unused(...)
 // For each argument inside the parentheses the pragma handler will produce

Did you consider using annot_typename for this? I ask because annot_typename handles general types (including decltype), and is already well-supported throughout the parser. It looks like you've checked for annot_decltype in several places, but others (such as the tentative parser) would be confused by this annotation token.

Index: include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- include/clang/Basic/DiagnosticSemaKinds.td	(revision 143902)
+++ include/clang/Basic/DiagnosticSemaKinds.td	(working copy)
@@ -3957,6 +3957,7 @@
   "conversion function from %0 to %1 invokes a deleted function">;
   
 def err_expected_class_or_namespace : Error<"expected a class or namespace">;
+def err_expected_class : Error<"expected a decltype of a class">;
 def err_missing_qualified_for_redecl : Error<
   "must qualify the name %0 to declare %q1 in this scope">;
 def err_invalid_declarator_scope : Error<

Could you provide a better error message here? Something that, at the very least, tells the user what the actual type is?

Also, enum types would also be fine in C++11, so the diagnostic isn't quite accurate.

Index: lib/Sema/SemaCXXScopeSpec.cpp
===================================================================
--- lib/Sema/SemaCXXScopeSpec.cpp	(revision 143902)
+++ lib/Sema/SemaCXXScopeSpec.cpp	(working copy)
@@ -676,6 +676,27 @@
                                      /*ScopeLookupResult=*/0, false);
 }
 
+bool Sema::ActOnCXXNestedNameSpecifierDecltype(CXXScopeSpec &SS,
+                                               const DeclSpec &DS,
+                                               SourceLocation ColonColonLoc) {
+  if (SS.isInvalid() || DS.getTypeSpecType() == DeclSpec::TST_error)
+    return true;
+
+  QualType Q = DS.getRepAsExpr()->getType();
+  if (!Q->getAs<TagType>()) {
+    Diag(DS.getTypeSpecTypeLoc(), diag::err_expected_class);
+    return true;
+  }
+
+  QualType T = BuildDecltypeType(DS.getRepAsExpr(), DS.getTypeSpecTypeLoc());
+  TypeLocBuilder TLB;
+  DecltypeTypeLoc DecltypeTL = TLB.push<DecltypeTypeLoc>(T);
+  DecltypeTL.setNameLoc(DS.getTypeSpecTypeLoc());
+  SS.Extend(Context, SourceLocation(), TLB.getTypeLocInContext(Context, T),
+            ColonColonLoc);
+  return false;
+}

This should probably check that the type specifier actually is a decltype (even if it's just an assertion). 

As an aside, I wonder if GCC supports typeof(blah)::foo, since typeof is the predecessor to decltype.

Index: lib/Parse/ParseDeclCXX.cpp
===================================================================
--- lib/Parse/ParseDeclCXX.cpp	(revision 143902)
+++ lib/Parse/ParseDeclCXX.cpp	(working copy)
@@ -633,14 +633,38 @@
 /// 'decltype' ( expression )
 ///
 void Parser::ParseDecltypeSpecifier(DeclSpec &DS) {
+  if (Tok.is(tok::kw_decltype)) {
+    AnnotateDecltypeSpecifier();
+  }
+  assert(Tok.is(tok::annot_decltype) && "Not a decltype specifier");
+  ExprResult Result = getExprAnnotation(Tok);
+  if (Result.isInvalid()) {
+    ConsumeToken();
+    DS.SetTypeSpecError();
+    return;
+  }
+  SourceLocation DecltypeLoc = ConsumeToken();
+
+  const char *PrevSpec = 0;
+  unsigned DiagID;
+  if (DS.SetTypeSpecType(DeclSpec::TST_decltype, DecltypeLoc, PrevSpec,
+                         DiagID, Result.get())) {
+    Diag(DecltypeLoc, DiagID) << PrevSpec;
+    DS.SetTypeSpecError();
+  }
+}

I'm not a big fan of this flow. It means that we go through the token-annotation logic even in the case where we're simply going to parse the decltype. Please factor this so that straight parsing of decltype() is simple, and we only annotate if we can't use the generated DeclSpec directly.

+void Parser::AnnotateDecltypeSpecifier() {
   assert(Tok.is(tok::kw_decltype) && "Not a decltype specifier");
 
+  // consume 'decltype'
   SourceLocation StartLoc = ConsumeToken();
-  BalancedDelimiterTracker T(*this, tok::l_paren);
-  if (T.expectAndConsume(diag::err_expected_lparen_after,
-                       "decltype", tok::r_paren)) {
-    return;
-  }
+  if (Tok.isNot(tok::l_paren))
+    Diag(Tok, diag::err_expected_lparen_after)
+      << "decltype"
+      << FixItHint::CreateInsertion(Tok.getLocation(), "("); 
+  else
+    ConsumeParen();
 
   // Parse the expression
 
Why aren't you using the BalancedDelimiterTracker here? It provides better error recovery and checking than your direct checks for l_paren/r_paren.

A couple notes on testing:

  - It would be good to make sure that decltype(T)::blah does the right thing during template instantiation
  - I don't see any tests where decltype shows up some place where we have to use tentative parsing (for the expression-or-declaration ambiguity)

Thanks for working on this!

	- Doug

> - David
> 
> On Sun, Nov 6, 2011 at 10:15 PM, David Blaikie <dblaikie at gmail.com> wrote:
>> Here's a patch that adds support for decltype in
>> nested-namespace-specifiers. I've included test cases for use of this
>> feature in both declarations and expressions (open to suggestions for
>> other contexts that might benefit from coverage).
>> 
>> I've also improved error recovery of the existing use of decltype (for
>> declarations) & these new uses by calling DeclSpec
>> setTypeSpecTypeError when errors are encountered while trying to parse
>> a decltype-specifier. There's probably still some error recovery to
>> improve, though, as I'm not currently doing anything with
>> ActOnCXXNestedNameSpecifierDecltype's return value as I'm not sure
>> what the best error recovery would be there. I suppose the
>> CXXScopeSpec should at least be failed.
>> 
>> - David
>> 
> <pr10127.diff>_______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits