This matches what MSVC does, and should make compiles faster by avoiding to unnecessarily emit a lot of code.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
No, this only affects the MS ABI.
MinGW doesn't seem to do this trick. For example, in this code, they will emit a definition for S<int>::Inner::f:
template <typename T> struct S { struct Inner { void f() {} }; }; template struct __declspec(dllimport) S<int>;
Looks great, thanks! A few minor questions below.
I verified that this has the same effect as my brute-force patch I tried locally.
Do we have test coverage for template class __declspec(dllexport) codecvt<char>; somewhere already?
lib/Sema/SemaTemplate.cpp | ||
---|---|---|
7382 ↗ | (On Diff #58365) | If there are multiple dllexports and dllimports on an explicit instantiation, cl.exe just silently picks the last one? |
7467 ↗ | (On Diff #58365) | 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) |
Thanks for the review!
Yes, that's covered by tests in CodeGenCXX/dllexport.cpp
lib/Sema/SemaTemplate.cpp | ||
---|---|---|
7382 ↗ | (On Diff #58365) | 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. |
7467 ↗ | (On Diff #58365) | I wanted to do this after PrevDecl_TSK has been defined, but we can move this a little earlier. I'll do that. |