hans accepted this revision. hans added a comment. This revision is now accepted and ready to land.
lgtm ================ 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: > > 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. > You're right; the current situation isn't ideal. I can clean that up in a > follow-up. Sounds good. https://reviews.llvm.org/D26657 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits