[PATCH] D20608: clang-cl: Treat dllimport explicit template instantiation definitions as declarations (PR27810, PR27811)

[Hans Wennborg via cfe-commits]

hans added a comment.

Thanks for the review!

In http://reviews.llvm.org/D20608#439480, @thakis wrote:

> Do we have test coverage for `template class __declspec(dllexport) codecvt<char>;` somewhere already?


Yes, that's covered by tests in CodeGenCXX/dllexport.cpp


================
Comment at: lib/Sema/SemaTemplate.cpp:7382
@@ +7381,3 @@
+      if (A->getKind() == AttributeList::AT_DLLExport)
+        DLLImport = false;
+    }
----------------
thakis wrote:
> If there are multiple dllexports and dllimports on an explicit instantiation, cl.exe just silently picks the last one?
The intention here was really just to reset the DLLImport value if it were set on line 7376, i.e. the attribute on the instantiation should override any attribute on the template.

I didn't even consider putting both dllimport and dllexport on the specialization. What does MSVC do? Well..

```
template <typename T> struct S { void f() {} };
template struct __declspec(dllimport) __declspec(dllexport) S<int>;
d:\src\tmp\a.cc(2) : warning C4141: 'dllexport' : used more than once 
```

That diagnostic seems a little bit confused, but the effect on codegen is even more so. S<int>::f() does not get defined here, which would suggest the instantiation def is treated as an instantiation decl on account of the dllimport. On the other hand, if I reference S<int>::f, it gets defined and *exported*. We probably don't want to reproduce this.

Clang will generally let dllexport take precedence over dllimport when they're on the same declaration, and has a nice warning for it, so let's do that here too.

================
Comment at: lib/Sema/SemaTemplate.cpp:7467
@@ +7466,3 @@
+    // The new specialization might add a dllimport attribute.
+    HasNoEffect = false;
+  }
----------------
thakis wrote:
> HasNoEffect is read two times before it's updated here. Is it intentional that you only change it after it's been read twice? If so, maybe add a comment why, since it looks not intentional else. (And make sure there's test coverage for setting it at the right time)
I wanted to do this after PrevDecl_TSK has been defined, but we can move this a little earlier. I'll do that.


http://reviews.llvm.org/D20608