[PATCH] Delay lookup of simple default template arguments under -fms-compatibility

[Reid Kleckner]

================
Comment at: include/clang/AST/Type.h:4059-4061
@@ -4058,5 +4058,5 @@
 ///
 /// DependentNameType represents a class of dependent types that involve a
 /// dependent nested-name-specifier (e.g., "T::") followed by a (dependent)
 /// name of a type. The DependentNameType may start with a "typename" (for a
 /// typename-specifier), "class", "struct", "union", or "enum" (for a
----------------
Richard Smith wrote:
> Please update this comment to indicate we also use `DependentNameType` in cases where we know we have a type but wish to defer the lookup for MS compatibility.
Done

================
Comment at: lib/Parse/ParseDecl.cpp:2764
@@ +2763,3 @@
+          getCurScope()->isTemplateParamScope() &&
+          DSContext == DSC_template_type_arg) {
+        TypeRep = Actions.ActOnDelayedDefaultTemplateArg(
----------------
Richard Smith wrote:
> David Majnemer wrote:
> > Perhaps move the `DSContext == DSC_template_type_arg` check before the calls to `getLangOpts()` ? It should be a little cheaper.
> I would think the most common reason this `if` condition is `false` is because `MSVCCompat` is `false` =) But I don't care either way.
OK, I'll hoist the DSC check.  I don't think we fail name lookup in template argument contexts very often, but I could be wrong.

================
Comment at: lib/Sema/SemaDecl.cpp:353-354
@@ +352,4 @@
+  DC = DC->getPrimaryContext();
+  NestedNameSpecifier *SubNNS = synthesizeCurrentNestedNameSpecifier(
+      Context, DC->getLookupParent(), NNS);
+  NamespaceDecl *ND = dyn_cast<NamespaceDecl>(DC);
----------------
Richard Smith wrote:
> Hmm. I don't think we actually ever need to recurse. Just walk up to the context into which we should look up the name, and build a single-level NNS for that.
OK.

================
Comment at: lib/Sema/SemaDecl.cpp:358
@@ +357,3 @@
+    return NestedNameSpecifier::Create(Context, SubNNS, ND);
+  else if (CXXRecordDecl *RD = dyn_cast<CXXRecordDecl>(DC))
+    return NestedNameSpecifier::Create(Context, SubNNS, RD->isTemplateDecl(),
----------------
Reid Kleckner wrote:
> Richard Smith wrote:
> > David Majnemer wrote:
> > > Perhaps `auto RD = dyn_cast<CXXRecordDecl(DC))` ?
> > Should we really ever use a class as the nested name specifier here? If I have a nested class (template) inside a class (template), should I look up a missing base specifier type in the outer class, or in the innermost enclosing namespace? (I would expect the latter is the right answer.)
> We need to be able to do lookup in enclosing RecordDecl scopes.  MSVC accepts this:
> 
>   struct Outer {
>     template <typename T = Baz> struct Foo {
>       static_assert(sizeof(T) == 4, "should get int, not double");
>     };
>     typedef int Baz;
>   };
>   typedef double Baz;
>   template struct Outer::Foo<>;
done

================
Comment at: lib/Sema/SemaDecl.cpp:358-359
@@ +357,4 @@
+    return NestedNameSpecifier::Create(Context, SubNNS, ND);
+  else if (CXXRecordDecl *RD = dyn_cast<CXXRecordDecl>(DC))
+    return NestedNameSpecifier::Create(Context, SubNNS, RD->isTemplateDecl(),
+                                       RD->getTypeForDecl());
----------------
Richard Smith wrote:
> David Majnemer wrote:
> > Perhaps `auto RD = dyn_cast<CXXRecordDecl(DC))` ?
> Should we really ever use a class as the nested name specifier here? If I have a nested class (template) inside a class (template), should I look up a missing base specifier type in the outer class, or in the innermost enclosing namespace? (I would expect the latter is the right answer.)
We need to be able to do lookup in enclosing RecordDecl scopes.  MSVC accepts this:

  struct Outer {
    template <typename T = Baz> struct Foo {
      static_assert(sizeof(T) == 4, "should get int, not double");
    };
    typedef int Baz;
  };
  typedef double Baz;
  template struct Outer::Foo<>;

================
Comment at: lib/Sema/SemaDecl.cpp:361
@@ +360,3 @@
+                                       RD->getTypeForDecl());
+  return SubNNS;
+}
----------------
Richard Smith wrote:
> Once we hit this, we should bail out entirely. (This can currently include local classes in the name specifier, which doesn't seem right.)
Bail out entirely, as in, return nullptr, or as in, fail the lookup?

================
Comment at: lib/Sema/SemaDecl.cpp:366
@@ +365,3 @@
+                                                SourceLocation NameLoc) {
+  Diag(NameLoc, diag::ext_ms_delayed_template_argument) << &II;
+
----------------
David Majnemer wrote:
> Perhaps a comment as to why we are emitting this diagnostic?
done

================
Comment at: test/SemaTemplate/ms-delayed-default-template-args.cpp:6-9
@@ +5,6 @@
+namespace test_basic {
+template <typename T = Baz> // expected-warning {{using the undeclared type 'Baz' as a default template argument is a Microsoft extension}}
+struct Foo { T x; };
+typedef int Baz;
+template struct Foo<>;
+}
----------------
Richard Smith wrote:
> Maybe also add a test for
> 
>   template<typename T> struct Something {};
>   template<typename T = Something<Bar>> struct SomethingElse;
> 
> ... and
> 
>   template<Something<Bar> *p> struct AnotherThing;
I assume Bar is always undeclared in these tests.

MSVC accepts your first test case, but Clang doesn't with my patch.  I'd need to find a way to delay instantiating Something<Bar> until later, which feels beyond the scope of this patch.

The second test is a negative test, right?  Both we and MSVC need to know the type of the non-type template parameter.

http://reviews.llvm.org/D3995