Thanks for the review! > I also have a few more tests you should use. For example currently sema > diagnoses dllimport redeclarations and drops this attribute, but class > template member functions would still be imported.
I'd like to look into the whole redeclaration business in a separate patch. It seems the rules are different for redeclaring classes and other things are different with respect to these attributes. ================ Comment at: lib/Sema/SemaDeclCXX.cpp:4375 @@ +4374,3 @@ + // FIXME: Warn: the returned class should probably be exported. + } + ---------------- Nico Rieck wrote: > You picked this from my old patch but I now think that this diagnostic > wouldn't buy much. MSVC doesn't do it, and it's perfectly valid to return for > example value objects. With this removed I just inlined this whole function > into the loop below. Sounds good to me. ================ Comment at: lib/Sema/SemaDeclCXX.cpp:4393 @@ +4392,3 @@ + + // FIXME: All bases must be exportable. MSVC doesn't seem to warn about + // this, but we should. We also need to propagate the attribute upwards to ---------------- Nico Rieck wrote: > I wonder what MS exactly means by that since it's valid to inherit from a > non-exported class or even an imported class. It's pretty confusing.. Their docs on http://msdn.microsoft.com/en-us/library/81h27t8c.aspx specifically say "All base classes of an exportable class must be exportable. If not, a compiler warning is generated." ================ Comment at: lib/Sema/SemaDeclCXX.cpp:4412 @@ +4411,3 @@ + // Instantiate non-default methods. + S.MarkFunctionReferenced(Class->getLocation(), MD); + } else if (!MD->isDeleted() && ---------------- Nico Rieck wrote: > When is this branch actually needed? It's needed for this case: template <typename T> struct __declspec(dllexport) U { void foo() {} }; struct __declspec(dllexport) V : public U<int> { }; to make us instantiate U<int>::foo(). ================ Comment at: lib/Sema/SemaDeclCXX.cpp:4413 @@ +4412,3 @@ + S.MarkFunctionReferenced(Class->getLocation(), MD); + } else if (!MD->isDeleted() && + (!MD->isTrivial() || MD->isCopyAssignmentOperator())) { ---------------- Nico Rieck wrote: > This check is redundant. Thanks! Removed. ================ Comment at: lib/Sema/SemaDeclCXX.cpp:4424 @@ +4423,3 @@ + } + } + } ---------------- Nico Rieck wrote: > This fails to export defaulted members for the MS abi (Mingw currently does > not export them). Thanks for catching that! http://reviews.llvm.org/D3877 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
