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

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added inline comments.


================
Comment at: include/clang/Sema/Sema.h:7494
 
+  /// \brief Make an existing DLL attribute on a class take effect.
+  void ActOnDLLAttr(ClassTemplateSpecializationDecl *Def,
----------------
hans wrote:
> Nit: I think `///` implies `\brief`, so we don't usually include it.
Cool, will drop.


================
Comment at: include/clang/Sema/Sema.h:7495
+  /// \brief Make an existing DLL attribute on a class take effect.
+  void ActOnDLLAttr(ClassTemplateSpecializationDecl *Def,
+                    InheritableAttr *Attr);
----------------
hans wrote:
> hans wrote:
> > I'd suggest making the function name more specific. It obviously doesn't apply to all DLL attributes, but only class templates.
> > 
> > Also, the "ActOn" methods in Sema are used as an interface to be called by the parser, so I would avoid that prefix.
> > 
> > Naming is hard :-/
> > 
> > Maybe `checkDllExportedTemplateSpecialization`
> also it should be in the `private:` section since it's just a helper. And maybe the "check" part is unnecessary? `dllExportClassTemplateSpecialization` might work.
Good point about the prefix. I'll do the rename. I intended to make it private; if it's in the wrong part of the header, that's an accident on my part :)


================
Comment at: lib/Sema/SemaTemplate.cpp:7439
+void Sema::ActOnDLLAttr(ClassTemplateSpecializationDecl *Def,
+                        InheritableAttr *Attr) {
+  // We reject explicit instantiations in class scope, so there should
----------------
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.


================
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
----------------
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).


https://reviews.llvm.org/D26657