[cfe-commits] [patch] PR10127/N3031 supporting decltype in nested-namespace-specifiers
David Blaikie
dblaikie at gmail.com
Sat Nov 19 23:27:33 PST 2011
Thanks again for the feedback/help.
>>> 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.
OK, I've done that & added a few test cases along the way for the
various cases that have been enabled (some of them weren't so easy to
figure out or didn't have existing test cases for the normal decltype
usage (without a nested-name-specifier)).
There's one case I fixed without actually needing any test cases
(ParseTentative - isExpressionOrTypeSpecifierSimple) because the cases
(expression or type name) get handled the same for decltype anyway.
>>> 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.
Ah, right. Though it's not an issue just yet - the message I've added
is only used for decltype issues anyway, but I've changed it to have
the necessary select. This'll be more use if/when we fix/improve the
original diagnostic as you were demonstrating in your talk at the dev
meeting.
> 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.
Nothing immediately - perhaps we could get away with dropping the "or
scoped enumeration" bit entirely? They appear like classes in code so
would it be reasonable to gloss over the distinction & just talk about
"namespace or class"?
>>> 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?
Probably, though I'm not sure exactly in what way(s). I actually had
to add another special case to my usage of BDT in the decltype parsing
- if you see the last test case I added (which involved 'decltype;') -
that would cause an assertion failure because the locations used in
the annotation token would include the ';' which was incorrect.
Instead in that case I needed to use the source location of the
decltype itself. The BDT should probably gave an invalid OpenLoc
rather than a location of an unconsumed token like that. Plus reliable
endloc reporting & possibly different error recovery rather than
searching so far forward (though that'll require some experimentation
to see what works best).
[I found one other problem with BalancedDelimiterTracker - it's
suggesting a fixit when the opening delimiter isn't present, but it
still returns failure (actually it's Parser::ExpectAndConsume that's
the culprit). Though simply returning false after Parser.cpp:173
produced a fair bit of test noise so I'll look into that separately at
some point]
>>> 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.
I found it in the end - see the changes related to ParseScope in
ParseDecl.cpp. The scope was only active within
ParseFunctionDeclarator, not the call to isCXXFunctionDeclarator which
is where the decltype was being parsed/annotated. This threw out the
scopeinfo.
I'm open to suggestions on how best to fix this - the fix I've got
here is just a basic demonstration of the problem. The code could be
restructured in a few ways (or potentially the Expr that the decltype
parses could be modified to have the right/matching scope info based
on where it was reconstituted but I'm not sure how much work that
would be)
>> 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 :)
I guess you probably meant to have the () in there too? (
ptr->~decltype(*ptr)() )
But yes, that's one of the other bits of N3031 we're missing - I plan
to fix all the cases given in that document before resolving the bug.
- David
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pr10127.diff
Type: text/x-patch
Size: 22121 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20111119/1918608e/attachment.bin>
More information about the cfe-commits
mailing list