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