[libcxx-commits] [PATCH] D97168: [libcxx] [test] Add -Wno-dllimport-static-field-def when building tests

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`?)

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97168/new/

https://reviews.llvm.org/D97168