[PATCH] D26657: [Sema] Respect DLL attributes more faithfully

[Hans Wennborg via Phabricator via cfe-commits]

hans added inline comments.


================
Comment at: lib/Sema/SemaTemplate.cpp:7439
+void Sema::ActOnDLLAttr(ClassTemplateSpecializationDecl *Def,
+                        InheritableAttr *Attr) {
+  // We reject explicit instantiations in class scope, so there should
----------------
smeenai wrote:
> hans wrote:
> > This function only applies to dllexport, not dllimport, so it would be good if the name reflected that, and maybe we could also add an assert to check for it.
> It's called in two places (the refactored original call for explicit instantiation declaration followed by explicit instantiation definition, and my new call for implicit instantiation followed by explicit instantiation definition). The dllexport guarantee only applies to the second one, right? I'll come up with a better name based on your suggestions in the other comment.
You're right, my mistake.


================
Comment at: lib/Sema/SemaTemplate.cpp:7710
+        (Context.getTargetInfo().getCXXABI().isMicrosoft() ||
+         Context.getTargetInfo().getTriple().isWindowsItaniumEnvironment())) {
+      // In the MS ABI, an explicit instantiation definition can add a dll
----------------
smeenai wrote:
> hans wrote:
> > Why the isWindowsItaniumEnvironment check? I'd expect checking for the MS ABI is sufficient?
> windows-itanium in general tries to stick to MSVC semantics for dllexport/import annotations (unlike Cygwin and MinGW which kinda do their own thing). This is consistent with the conditional for the previous case (lines 7691 to 7693 in this diff).
Oh I see, this seems to be a new thing, starting with e.g. r284288.

Seems fine then, but I'm a little worried that we're adding another variable into the matrix here. IIRC, we key dll attribute behaviour off `getCXXABI().isMicrosoft()` in lots of places.


https://reviews.llvm.org/D26657