[libcxx-commits] [PATCH] D97168: [libcxx] [test] Add -Wno-dllimport-static-field-def when building tests
Martin Storsjö via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Mon Feb 22 11:52:51 PST 2021
mstorsjo added subscribers: hans, rnk.
mstorsjo added a comment.
In D97168#2579077 <https://reviews.llvm.org/D97168#2579077>, @ldionne wrote:
> Is this really the right fix? I think Clang is warning about an actual issue, i.e. that we're defining `npos` in the test file when we shouldn't be? If we just silence this warning in the test suite, it'll fire when people build against libc++ on Windows.
Yeah, if we're running into this warning here, technically we would warn on the same when included in user code. In practice we don't, as warnings from system headers (as the libc++ headers are at that point) normally are muted though.
(We're not defining `npos` in the test files, it just warns about the declarations in the headers.)
> What's the Microsoft recommendation for using `dllimport` on static data members?
I'd like to summon @rnk and @hans here. In my experience, at least the testsuite runs fine in DLL configurations if we revert rG2d8f23f571635c1fb983b40c4c2548716a5b65b6 <https://reviews.llvm.org/rG2d8f23f571635c1fb983b40c4c2548716a5b65b6> and rG59919c4d6b6370da7133bbca0d31844e21646bb1 <https://reviews.llvm.org/rG59919c4d6b6370da7133bbca0d31844e21646bb1>.
As far as I can see it, the first one adds `_LIBCPP_FUNC_VIS`, with this explanation:
> [libc++] Explicitly mark basic_string<...>::npos with default visibility.
> This ensures that the version compiled into the library isn't accidentally hidden.
I presume this is a fix for non-windows systems. (It doesn't come with references to review or other longer elaborations.) And the other one adds a second matching `_LIBCPP_FUNC_VIS` (in order not to break windows DLL builds altogether).
Is the correct fix to just omit the `_LIBCPP_FUNC_VIS` from the `npos` declaration (in both instances) in windows configurations? (I guess we don't yet have any macro like `_LIBCPP_FUNC_VIS_EXCEPT_ON_WINDOWS`?)
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
More information about the libcxx-commits