[cfe-commits] r146480 - in /cfe/trunk: include/clang/Basic/DiagnosticSemaKinds.td lib/Sema/SemaDecl.cpp test/CXX/dcl.decl/dcl.meaning/p1-0x.cpp test/CXX/expr/expr.prim/expr.prim.general/p8-0x.cpp

[Douglas Gregor]

On Dec 14, 2011, at 11:11 AM, David Blaikie wrote:

> On Wed, Dec 14, 2011 at 7:58 AM, Douglas Gregor <dgregor at apple.com> wrote:
>> 
>> On Dec 13, 2011, at 12:03 AM, David Blaikie wrote:
>> 
>>> Author: dblaikie
>>> Date: Tue Dec 13 02:03:36 2011
>>> New Revision: 146480
>>> 
>>> URL: http://llvm.org/viewvc/llvm-project?rev=146480&view=rev
>>> Log:
>>> Disallow decltype in qualified declarator-ids.
>>> 
>>> Added:
>>>    cfe/trunk/test/CXX/dcl.decl/dcl.meaning/p1-0x.cpp
>>> Modified:
>>>    cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
>>>    cfe/trunk/lib/Sema/SemaDecl.cpp
>>>    cfe/trunk/test/CXX/expr/expr.prim/expr.prim.general/p8-0x.cpp
>>> 
>>> Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=146480&r1=146479&r2=146480&view=diff
>>> ==============================================================================
>>> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
>>> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Tue Dec 13 02:03:36 2011
>>> @@ -1144,6 +1144,8 @@
>>> def warn_cxx98_compat_decltype : Warning<
>>>   "'decltype' type specifier is incompatible with C++98">,
>>>   InGroup<CXX98Compat>, DefaultIgnore;
>>> +def err_decltype_in_declarator : Error<
>>> +    "'decltype' cannot be used to name a declaration">;
>>> 
>>> // C++11 auto
>>> def warn_cxx98_compat_auto_type_specifier : Warning<
>>> 
>>> Modified: cfe/trunk/lib/Sema/SemaDecl.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDecl.cpp?rev=146480&r1=146479&r2=146480&view=diff
>>> ==============================================================================
>>> --- cfe/trunk/lib/Sema/SemaDecl.cpp (original)
>>> +++ cfe/trunk/lib/Sema/SemaDecl.cpp Tue Dec 13 02:03:36 2011
>>> @@ -3203,6 +3203,16 @@
>>>          (S->getFlags() & Scope::TemplateParamScope) != 0)
>>>     S = S->getParent();
>>> 
>>> +  if (NestedNameSpecifierLoc SpecLoc =
>>> +        D.getCXXScopeSpec().getWithLocInContext(Context)) {
>>> +    while (SpecLoc.getPrefix())
>>> +      SpecLoc = SpecLoc.getPrefix();
>>> +    if (dyn_cast_or_null<DecltypeType>(
>>> +          SpecLoc.getNestedNameSpecifier()->getAsType()))
>>> +      Diag(SpecLoc.getBeginLoc(), diag::err_decltype_in_declarator)
>>> +        << SpecLoc.getTypeLoc().getSourceRange();
>>> +  }
>>> +
>> 
>> The code here looks correct (thanks!), but please:
>> 
>>  (1) Move it down to where we do the other checks on the nested-name-specifier (with the comment that starts with "C++ 7.3.1.2p2")
> 
> Ah, sure thing. Moving this down puts it under a "Ss.isSet()"
> condition, so my test case changes (we don't flag this error if the
> CXXScopeSpec is already invalid).

That's good. If there's already an error in the nested-name-specifier, it's not beneficial to complain more about it.

> Should this condition be changed
> above (line 3298) be changed from "if (!D.getCXXScopeSpec().isSet())"
> to "if (D.getCXXScopeSpec().isEmpty())" ? or would that cause more
> confusion/error noise than would be helpful. I suppose the other
> errors in this block only work if name lookup succeeds, and doing name
> lookup on an invalid scope spec isn't going to be useful (in which
> case "if (D.getCXXScopeSpec().isValid())" might be the right thing).
> Alternatively, my new test could be moved down outside the
> if(SS.isSet())/else block but it'd still be close to the related
> checks.

We don't want to do name lookup on invalid nested-name-specifiers.

> [side note: CXXScopeSpec.isSet is deprecated, but it has a simpler
> implementation that "isValid", yet /seems/ to provide the same
> guarantee - isSet checks the ScopeRep != 0, isValid tests for a
> non-empty range and ScopeRep != 0. Is it possible to have a non-null
> scoperep and an invalid/empty source range? If not then isSet and
> isValid should use the same (simpler) implementation]

IIRC, it's the other way around. It's possible to have a NULL scope rep (because the nested-name-specifier was ill-formed) but a non-empty source range (because we still want to know where the sources were).

>>  (2) Add a reference to the C++11 spec, quoting the paragraph that introduces this restriction.
> 
> I quoted the relevant sentence from that paragraph (I assume you
> didn't want the whole paragraph, it's somewhat long-winded).
> 
> Interestingly there's a note in that paragraph (about what
> nested-name-specifiers are allowed in declarator-ids) that says "If
> the qualifier is the global :: scope resolution operator, the
> declarator-id refers to a name declared in the global namespace
> scope." Yet clang disallows this construct. I can't see any reason for
> it to be a useful/valid construct - but is that just a spec
> bug/oversight, or am I (& clang) missing something?


8.3p1 says that you can only use a qualified-id when defining something outside its scope, and of course you can only define something in an outer namespace scope. Note that GCC/EDG/Clang all diagnose this the same way.

	- Doug