Fix for Bug 14644 - clang confuses scope operator for global namespace giving extra qualification on member

[ryee88]

Summary

Bug 14644 - clang confuses scope operator for global namespace giving extra qualification on member

This is a fix for this bug: https://llvm.org/bugs/show_bug.cgi?id=14644

Essentially the diagnostic needs to distinguish between a "global-specifier with no nested-name-specifier" and nested-name-specifier (everything else) in order to provide a more helpful error message.

[ryee88]

Ping-- I think this review got missed.

[ryee88]

Added unit tests.

[ryee88]

Messed up the diff. Uploading it again.

[ryee88]

Anyone care to take a look?

[ryee88]

Anyone want to pick up this review?

[aaron.ballman]

lib/Sema/SemaDecl.cpp

3813

Can drop the N-paper reference, and replace "3.4.4, section 3" with [basic.lookup.elab]p3

3814

Ah, you're referring to text in the example; I think the reference above should actually be to [dcl.type.elab]

3815

Can drop the clang::

3821

Should remove the diagId = from the else clause.

[dblaikie]

Not sure I fully understand the issue with the existing diagnostic/change in wording.

'::' is a nested name specifier, even if it's a degenerate case, I think - so the old wording doesn't seem wrong, as such (& the new text seems correct in the other cases too - should we just switch to that in general rather than having two cases?)

[ryee88]

lib/Sema/SemaDecl.cpp

3813

I'm going to re-organize these comments and move the conditional because really all these comments apply too.

3815

ok

3821

whoops. will do.

[ryee88]

In D16965#377864, @dblaikie wrote:

Not sure I fully understand the issue with the existing diagnostic/change in wording.

'::' is a nested name specifier, even if it's a degenerate case, I think - so the old wording doesn't seem wrong, as such (& the new text seems correct in the other cases too - should we just switch to that in general rather than having two cases?)

Yes that's the basically the issue raised in the bug report. The error wording for the degenerate nested name specifier is unclear. I'll tinker around with a generic wording.

[rsmith]

Note that the diagnostic wording has already been fixed compared with the diagnostic reported in that bug report. Do we actually need more changes here?

[ryee88]

Yeah I don't think it's worth it to modify the general case wording to to explicitly call out the degenerate global nested-name-specifier. It'll cause more confusion for the general case.

At this point, do we just close the bug and keep the new unit tests?

[ryee88]

Removed unnecessary code changes. This patch contains a unit test to detect a regression of this global name specifier issue.

include/clang/Basic/DiagnosticSemaKinds.td

 def err_standalone_class_nested_name_specifier : Error<
   "forward declaration of %select{class|struct|interface|union|enum}0 cannot "
   "have a nested name specifier">;
+def err_standalone_class_specifier : Error<
+  "forward declaration of qualified %select{class|struct|interface|union|enum}0 "
+  "not allowed">;
 def err_typecheck_sclass_func : Error<"illegal storage class on function">;
 def err_static_block_func : Error<
   "function declared in block scope cannot have 'static' storage class">;

lib/Sema/SemaDecl.cpp

   if (Tag && SS.isNotEmpty() && !Tag->isCompleteDefinition() &&
       !IsExplicitInstantiation && !IsExplicitSpecialization &&
       !isa<ClassTemplatePartialSpecializationDecl>(Tag)) {
+    
+    // Per C++ standard [n3485] 3.4.4 Elaborated type specifiers, section 3:
+    // "Cannot introduce an qualified".
+    // A clang::NestedNameSpecifier can represent many kinds of specifiers.
+    // A global-specifier with no nested-name-specifier requires a different
+    // diagnostic from a nested-name specifier.
+    unsigned diagId = ( SS.getScopeRep()->getKind() == NestedNameSpecifier::Global &&
+      !SS.getScopeRep()->getPrefix() )
+      ? diag::err_standalone_class_specifier
+      : diagId = diag::err_standalone_class_nested_name_specifier;
+
     // Per C++ [dcl.type.elab]p1, a class declaration cannot have a
     // nested-name-specifier unless it is an explicit instantiation
     // or an explicit specialization.

     // obvious intent of DR1819.
     //
     // Per C++ [dcl.enum]p1, an opaque-enum-declaration can't either.
-    Diag(SS.getBeginLoc(), diag::err_standalone_class_nested_name_specifier)
-        << GetDiagnosticTypeSpecifierID(DS.getTypeSpecType()) << SS.getRange();
+    Diag(SS.getBeginLoc(), diagId)
+
     return nullptr;
   }