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

[David Blaikie]

> Some comments follow…

Thanks for the feedback.

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

No, I hadn't considered that - but at first blush I'd be concerned
that this might end up being /too/ permissive, but I'll have a go
mocking it up that way, it'll probably be tidier but I won't be sure
what other scenarios might be accidentally accepted.

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

Sure - I was going off the (admittedly rather vague) existing message
above. I've rephrased this to, for example:

    'decltype(int())' (aka 'int') is not a namespace, class, or scoped
enumeration

I think it'd be helpful to use this diagnostic for the existing case
above. It comes up most easily if you have a typedef that resolves to
int for example:

    namespace foo { typedef int bar; }

    foo::bar::baz i; // "expected class or namespace" (^ points to the
'b' in 'bar')

& you don't get told what foo::bar is. Though my version is a bit more verbose.

> 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).

Added an assertion.

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

GCC 4.6 doesn't seem to support decltype or typeof in nested name
specifiers. I don't have 4.7 on hand to compare it.

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

I was modeling this off the behavior I saw in
Parser::ParseDeclarationSpecifiers which always annotates the
CXXScopeToken rather than using it directly, presumably (or so I
thought) out of convenience of merging the two cases (where it was
called without an annotation token or with one), though it did feel a
bit questionable.

I've switched this back to the original implementation (though still
with better error recovery using SetTypeSpecError) & only annotating
when necessary.

> +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.

Originally I'd pulled this out/removed BalancedDelimiterTracker so
that I could avoid consuming the trailing ')' & just mutate that into
the annotation token I required. But since I ended up having to
account for the case where I needed to insert a token to be the
annotation token anyway (when recovering from a lack of ')') I assume
I might as well just do that in all cases & keep with the old
implementation.

One problem I have using the BalancedDelimiterTracker is getting the
last token it consumes so I can set the annotation token's end
correctly. Though I think the only case where the
BlancedDelimiterTracker doesn't set and end location is when it
doesn't find one & goes all the way to eof. I haven't thought too much
about what that situation means/how it comes out in my current code or
how it should come out.

[As for error recovery, BalancedDelimiterTracker does do the extra
legwork to handle nesting quantities which is certainly valuable, but
its error recovery is a bit different (& without a FixIt). It skips
everything up to the next matching token, my error recovery was to
insert the missing token - I wonder which is more likely to be
correct? (it'd also be kind of nice to be able to find the location of
the previous token & insert the fixit for the missing paren after the
previous token rather than before the next token. But that's a minor
whitespace improvement ("decltype(x y;" -> "decltype(x) y;" rather
than "decltype(x )y;").]

> A couple notes on testing:
>
>  - It would be good to make sure that decltype(T)::blah does the right thing during template instantiation

Good call - this didn't work immediately for dependent names. I've
added a simple test case & fixed the bug I had blocking this scenario.
(lib/Sema/SemaCXXScopeSpec.cpp:688 - was failing if the expression was
not a TagType. I've made it more permissive to allow for any dependent
type or non-dependent tag types)

>  - 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)

I've added a test case for this & made a small change to fix the
scenario - but this change has somehow regressed a name mangling test
related to templates and decltype usage... I haven't had time to
investigate this in great detail but thought I'd show it as-is in case
you or anyone else has some ideas about what's going on here.

Also, it seems currently that we don't support decltype in expressions
(looks like it's half done - Parser::isCXXSimpleTypeSpecifier has
decltype, but ParseCXXSimpleTypeSpecifier has a FIXME to add it). If I
switch to using a typename annotation this might come out basically
for free, perhaps.

I haven't prototyped an implementation using typename yet, but if I
get a chance (and/or if I fix the name mangling issue) I'll send an
update.

Thanks,
- David
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pr10127.diff
Type: text/x-diff
Size: 17260 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20111114/43ab024b/attachment.diff>