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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
