mstorsjo added inline comments.
================ Comment at: lib/CodeGen/ItaniumCXXABI.cpp:2990-2993 + bool IsMinGW = + CGM.getContext().getTargetInfo().getTriple().getEnvironment() == + llvm::Triple::GNU && + CGM.getContext().getTargetInfo().getTriple().isOSWindows(); ---------------- compnerd wrote: > This is equivalent to: > `CGM.getContext().getTargetInfo().getTripleInfo().isWindowsGNUEnvironment()` Oh, indeed. Will change that before pushing. Thanks! ================ Comment at: lib/CodeGen/ItaniumCXXABI.cpp:2994 + CGM.getContext().getTargetInfo().getTriple().isOSWindows(); + // MinGW always uses LinkOnceODRLinkage for type info. + if (RD->isDynamicClass() && !IsMinGW) ---------------- compnerd wrote: > mstorsjo wrote: > > mstorsjo wrote: > > > Thanks for having a look at this! > > > > > > When giving this a second thought now, should we perhaps skip the check > > > for `Environment == GNU` altogether? Or should `xxx-windows-itanium` be > > > different by restricting this behaviour strictly to GNU (does it matter)? > > > > > > And given the existing test case that you added with the previous form of > > > this code, and the new test I'm adding here, are there any other odd > > > corner cases I should check? > > To rephrase; previously, the comment talked about mingw specifically, but > > the code only checked for the dllimport attribute. So perhaps I should drop > > the check for GNU and just check for windows? > Im not 100% certain off the top of my head, but I believe that > `windows-itanium` should actually import the typename, so this should be okay. Ok then, keeping it specific to MinGW.. https://reviews.llvm.org/D37206 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
