rnk added a comment. I think this generally seems right, but we should make sure our behavior is more consistent in the case of a template definition.
================ Comment at: lib/Sema/SemaDecl.cpp:5570-5571 @@ -5565,4 +5569,4 @@ // exceptions being inline function definitions, local extern declarations, - // and qualified friend declarations. - // NB: MSVC converts such a declaration to dllexport. + // qualified friend declarations or special MSVC extension: in the last case, + // the declaration is treated as if it were marked dllexport. bool IsInline = false, IsStaticDataMember = false, IsQualifiedFriend = false; ---------------- This comment is a run-on, maybe try this: If a redeclaration drops the dllimport attribute, we usually ignore the previous attribute with a warning. If the redeclaration is an inline definition, a local extern declaration, or a qualified friend declaration, we do nothing. If this declaration is a plain, non-inline definition, then we translate dllimport to dllexport and warn the user. ================ Comment at: lib/Sema/SemaDecl.cpp:5595 @@ +5594,3 @@ + NewDecl->addAttr(::new (S.Context) DLLExportAttr( + NewImportAttr->getRange(), S.Context, + NewImportAttr->getSpellingListIndex())); ---------------- In CodeGen, when we make decisions about the actual LLVM IR dll storage class, we should call Decl::getMostRecentDecl, and look at the attributes on that. I think we can defer fixing this. You can see instances of the same problem below where we drop dllimport from previous declarations. We shouldn't be doing that. ================ Comment at: lib/Sema/SemaDecl.cpp:5600-5602 @@ +5599,5 @@ + diag::warn_redeclaration_without_attribute_prev_attribute_ignored) + << NewDecl << OldImportAttr; + S.Diag(OldDecl->getLocation(), diag::note_previous_declaration); + S.Diag(OldImportAttr->getLocation(), diag::note_previous_attribute); + OldDecl->dropAttr<DLLImportAttr>(); ---------------- Use DLLExportAttr::CreateImplicit or Attr->setImplicit(true). ================ Comment at: test/Sema/dllimport.c:52 @@ +51,3 @@ +__declspec(dllimport) int GlobalDeclInit; // expected-note{{previous declaration is here}} +#ifdef MS +// expected-warning@+4{{'GlobalDeclInit' redeclared without 'dllimport' attribute: 'dllexport' attribute added}} ---------------- We've already got one test for the difference between MSVC and GNU mode. I think the rest of these test cases can skip the ifdefs and look for the common prefix instead, like this: // expected-warning {{'GlobalDeclInit' redeclared without 'dllimport' attribute}} ================ Comment at: test/SemaCXX/dllimport.cpp:62 @@ +61,3 @@ +__declspec(dllimport) int GlobalDeclInit; // expected-note{{previous declaration is here}} +#ifdef MS +// expected-warning@+4{{'GlobalDeclInit' redeclared without 'dllimport' attribute: 'dllexport' attribute added}} ---------------- ditto, after the first MS-specific diagnostic test case we don't need the duplication ================ Comment at: test/SemaCXX/dllimport.cpp:179 @@ -140,1 +178,3 @@ +template <typename T> +int ExternVarTmplDeclInit = 1; ---------------- Can you check with MSVC 2015 update 2 actually does with definitions of dllimport variable templates? I bet it doesn't export them. ================ Comment at: test/SemaCXX/dllimport.cpp:1137 @@ -1017,1 +1136,3 @@ +template <typename T> +void ImportClassTmplMembers<T>::normalDef() {} #ifdef GNU ---------------- I'm pretty sure MSVC considers all free function templates to be 'inline', i.e. they put them in comdats. I doubt MSVC exports these. Can you verify what it does? http://reviews.llvm.org/D18953 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits