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

3803 ↗

(On Diff #49866)

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

3804 ↗

(On Diff #49866)

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

3805 ↗

(On Diff #49866)

Can drop the clang::

3811 ↗

(On Diff #49866)

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

3803 ↗

(On Diff #49866)

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

3805 ↗

(On Diff #49866)

ok

3811 ↗

(On Diff #49866)

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.

test/Parser/cxx-class.cpp

     // expected-error{{unknown type name 'UnknownType'}}
 }
 
-namespace nns_decl {
-  struct A {
-    struct B;
+namespace NestedNameSpecifiers {
+  struct OuterStruct {
+    struct InnerStruct;
   };
-  namespace N {
-    union C;
+  namespace UnionOuterNamespace {
+    union UnionInner;
   }
-  struct A::B; // expected-error {{forward declaration of struct cannot have a nested name specifier}}
-  union N::C; // expected-error {{forward declaration of union cannot have a nested name specifier}}
+  class OuterClass{
+    class InnerClass;
+  };
+
+  struct OuterStruct::InnerStruct; // expected-error {{forward declaration of struct cannot have a nested name specifier}}
+  struct ::NestedNameSpecifiers::OuterStruct::InnerStruct; // expected-error {{forward declaration of struct cannot have a nested name specifier}}
+
+  union UnionOuterNamespace::UnionInner; // expected-error {{forward declaration of union cannot have a nested name specifier}}
+  union ::NestedNameSpecifiers::UnionOuterNamespace::UnionInner; // expected-error {{forward declaration of union cannot have a nested name specifier}}
+
+  class OuterClass::InnerClass; // expected-error {{forward declaration of class cannot have a nested name specifier}}
+  class ::NestedNameSpecifiers::OuterClass::InnerClass; // expected-error {{forward declaration of class cannot have a nested name specifier}}
 }
 
+// Testing the global "nested" name qualifier
+class GlobalSpecifierOuterClass {class InnerClass;};
+class ::GlobalSpecifierOuterClass; // expected-error {{forward declaration of class cannot have a nested name specifier}} expected-warning {{extra qualification on member 'GlobalSpecifierOuterClass'}}
+class GlobalSpecifierOuterClass;
+// specializations of qualified type introduction?
+class GlobalSpecifierOuterClass::InnerClass; // expected-error {{forward declaration of class cannot have a nested name specifier}}
+class ::GlobalSpecifierOuterClass::InnerClass; // expected-error {{forward declaration of class cannot have a nested name specifier}}
+
+
 // PR13775: Don't assert here.
 namespace PR13775 {
   class bar