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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
