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

[Douglas Gregor]

On Nov 14, 2011, at 8:10 AM, David Blaikie wrote:

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

I'm fine with keeping annot_decltype. It's super-specific, so just make sure you're auditing the various places where we have to deal with, e.g., the 'decltype' keyword or the annot_typename token so you catch all of the places 'decltype' can show up.

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

It's definitely an improvement. We only want that 'or scoped enumeration' bit if we're in C++11.

I wonder if there's a phrase that could describe these kinds of entities with a scope without having to enumerate the examples? Nothing comes to mind yet.

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

Okay! Let's just stick with decltype only.

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

Thanks.

>> +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;").]

Should BalancedDelimiterTracker be improved to help handle this case?

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

Okay.

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

I don't know off-hand what could have broken.

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

Oops! Should be an easy fix.

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

Okay! I think the annot_typename idea was a bad one on my part; annot_decltype seems to be the way to go.

I noticed that there's another (completely separable and not necessarily your problem) decltype-related issue, which is that we don't parse:

template<typename T>
void f(T *ptr) {
  ptr->~decltype(*ptr);
}

at all. Should be trivial following your changes :)

	- Doug