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

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

Apologies for the delay. I was out last week.

In https://reviews.llvm.org/D26657#602083, @smeenai wrote:

> General coding style question. Over here, I'm creating a local helper function. However, that helper needs to access member functions of Sema, which is why I made it a private member function right now, which also unfortunately makes the change somewhat non-local (since the header file needs to be modified as well). I can think of two alternatives:
>
> - Define a local helper lambda (which will capture `this`) instead of a private member function.
> - Define a local static helper function and pass the Sema instance as a parameter so that it can call member functions on it.
>
>   Would either of those be preferable to what I currently have? I'm pretty new when it comes to llvm/clang changes, so stylistic feedback is greatly appreciated.


I usually prefer a static helper function and passing the Sema instance if it's possiblem, but that requires the Sema members we need to access to be public. If that's not the case, adding the helper as a member function is fine.



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


================
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);
----------------
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`


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


================
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
----------------
Why the isWindowsItaniumEnvironment check? I'd expect checking for the MS ABI is sufficient?


https://reviews.llvm.org/D26657