mstorsjo wrote: > I too had trouble understanding this change based on the description. Could > you give a concrete example of how mingw and msvc disagree on where to put > the attribute, and explain how this pr changes things? > > Taking a step back, how will this simplify libc++'s visibility annotations? > Don't they still need to be compatible with both mingw and msvc, which we > don't control?
Can you respond to these questions here, they're essential to the motivation of this change IMO. Also, won't the potential simplification in libc++ essentially just amount to this? ```diff diff --git a/libcxx/include/__config b/libcxx/include/__config index 38c47e8d45c8..4102e41ebe23 100644 --- a/libcxx/include/__config +++ b/libcxx/include/__config @@ -341,13 +341,8 @@ typedef __char32_t char32_t; # define _LIBCPP_OVERRIDABLE_FUNC_VIS # define _LIBCPP_EXPORTED_FROM_ABI # elif defined(_LIBCPP_BUILDING_LIBRARY) -# if defined(__MINGW32__) -# define _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS __declspec(dllexport) -# define _LIBCPP_CLASS_TEMPLATE_INSTANTIATION_VIS -# else -# define _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS -# define _LIBCPP_CLASS_TEMPLATE_INSTANTIATION_VIS __declspec(dllexport) -# endif +# define _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS __declspec(dllexport) +# define _LIBCPP_CLASS_TEMPLATE_INSTANTIATION_VIS __declspec(dllexport) # define _LIBCPP_OVERRIDABLE_FUNC_VIS __declspec(dllexport) # define _LIBCPP_EXPORTED_FROM_ABI __declspec(dllexport) # else ``` The downside is that it would be _massively_ much more confusing to use Clang, as a developer, if it silently accepts `dllexport` in the wrong place, if it just doesn't do anything with it there. As a developer, you'd have to essentially guess and try blindly until you stick it in the right place where it actually does the right thing, if Clang doesn't tell you about cases where it's silently ignored. https://github.com/llvm/llvm-project/pull/133699 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
