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

Douglas Gregor dgregor at apple.com
Sat Dec 3 10:58:11 PST 2011


On Nov 19, 2011, at 11:27 PM, David Blaikie wrote:
>>>> 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.

Right.

>> 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"?

I'd rather go more general, e.g., "qualified lookup into type %0 that can not have members".

>>> 
>>> [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]

Okay. It's not important to tackle here, but I'd like to make BDT more robust/useful for these sorts of things in the future.

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

Very nice find!

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

I think your fix is fine, but please add an assertion in ParseFunctionDeclarator to make sure that the currently-active scope is actually a function prototype scope.

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

Yes, that's what I meant.

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


Great. It doesn't have to be in this patch, of course.

This is looking great. With the requested assertion (and assuming that doesn't break anything), please go ahead and commit!

	- Doug



More information about the cfe-commits mailing list