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

[David Blaikie]

On Sat, Dec 3, 2011 at 10:58 AM, Douglas Gregor <dgregor at apple.com> wrote:
>
> 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".

Hmm - maybe, except that it isn't necessarily a type (it might be a
namespace scoped variable). In any case I might leave this up to you
to change the wording when you checkin the diagnostic changes you
demonstrated in your LLVM dev meeting talk.

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

Yeah - that's what I was angling at (not trying to fix it in this
checkin - but that there's certainly things to be looked at). I'll
have a play around with it.

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

Thanks - took a bit of concerted probing.

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

Fair enough - I added the assert.

I'm a little concerned that not doing the fancy thing I described
above (modifying the Expr when it's loaded from the annotation -
walking it to adjust the scope nesting values, etc) could lead to
similarly subtle bugs in the future when similar situations arise (it
is a bit subtle that an "isFoo" function ends up changing the token
stream/parsing something early, etc). But I'll certainly defer to your
judgment in this case as to what's the appropriate approach - it'll
just teach me to be aware of the possibility of such subtleties in
other parts of the codebase.

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

Added the assertion, fixed an 80 col violation* in the warning tblgen
file, and submitted as r145785. Thanks a bunch for all your help
reviewing this.

* speaking of 80 col violations - what's the rule for columns in test
files, a lot of them tend to violate 80 cols, but I'm happy to try to
wrap at 80 cols if that's preferred (I suppose it is preferred, just
not strongly adhered to)

Next on the list is decltype as a simple name specifier (
decltype(std::string())("foo") + "bar"), decltype in ctor initializer
lists (to match the decltype in the base class list), decltype in dtor
invocations as you showed above... and anything else I can find in
N3031. (& that stuff in the BalancedDelimiterTracker)

Thanks again,
- David