In D141748#4060636, @efriedma wrote:

The explanation makes sense.

Can we make this specifically apply to _Interlocked* functions, instead of all atomics on Windows targets? I'd prefer not to impose this performance penalty on other users of atomics if we can avoid it.

There should probably be a note here that this test isn't testing
standard guarantee; it's testing that Howard's dining philosophers
rebooted algorithm is implemented.

In D87974#2439043, @jwakely wrote:

In D87974#2438723, @zoecarver wrote:

In D87974#2438682, @BillyONeal wrote:

Are they actually the same, with the same handling of corner cases like unions and tail padding?
There's more to this than just the name, and if they aren't the same, it seems better to have two names.

They are both implementing the same C++ feature, with the same desired semantics of zeroing out any bits in the object representation that aren't in the value representation. If they differ, one or the other would have a bug.

Do they support non-trivially copyable types? That isn't required for the atomic compare exchange feature, but is relevant for a feature exposed to users. What about extensions like zero-sized arrays or C99 flexible array members?

Are they actually the same, with the same handling of corner cases like unions and tail padding?
There's more to this than just the name, and if they aren't the same, it seems better to have two names.

In D87974#2432793, @zoecarver wrote:

So, it looks like GCC already uses __builtin_clear_padding and MSVC already uses __builtin_zero_non_value_bits. This patch (obviously) is currently implementing __builtin_zero_non_value_bits but, I had planned to update it to use __builtin_clear_padding. Maybe that's not the best course of action, though.

We should all try to agree on _one_ name. CC @BillyONeal @jwakely thoughts?

BTW ThreadPoolExecutor has undefined behavior since there's a detached thread touching the standard library when the program exits violating [basic.start.term]/6. Detached threads are almost never safe.

Thanks!

@ldionne I think you're correct, though the test there is more complex than it needs to be (because the ==Count cases are the only ever encountered here).

This was https://reviews.llvm.org/D61366

https://github.com/llvm/llvm-project/commit/45f630d729e2cce044ed48e6eaf4b8e61e06fede Committed

@BillyONeal has proposed a more reasonable set of requirements

It looks like someone already fixed this.

I have not run either through tests yet...

That's worse; it replaced 2n assignments with n assignments, n constructions, and n destructions. Interestingly, assigning over __saved is an optimization I should put into MSVC.

(In fact I observe many patterns in this review like:

In D65249#1603335, @jfb wrote:

@rnk: how about I add a bit of code that wraps aligned_storage on all platforms except MSVC (where I'd implement it as Billy suggests). That would mean updating all the uses of aligned_storage to this LLVM one, but that's not a big deal.

@BillyONeal do you know if 19.11 has the aligned_storage issue on x86?

In D61366#1569151, @mclow.lists wrote:

Is there a reason this hasn't been committed?

In D61364#1502200, @EricWF wrote:

From Billy and my last discussion, I think we came to the agreement that it's not clear exactly what the "standard behavior" is.

In D61364#1502172, @ldionne wrote:

Do you know *why* the tests are failing with libc++? I see this overload for insert and it seems like it should be a better match?

In D61366#1502170, @Quuxplusone wrote:

you are indeed allowed to move the elements

In D61366#1502073, @EricWF wrote:

I'm not sure I agree with your design decision, but this patch LGTM.

In D59676#1498167, @aganea wrote:

@BillyONeal do you think your new parallel algorithm(s) will give a better throughput?

Also fixed vector<bool> test.

Committed r357546

Committed r357545

Tim Song suggests that http://eel.is/c++draft/string.require#2 indicates that basic_string actually does need to provide the strong guarantee here. While I think that wording is a mess I think I'll fix our basic_string to do that instead of that part of this change.

Fixed misspelled test macro.

Fix asserts for the strong EH guarantee.

f you happen to have pointers to the APIs to use here… :)

I think this is a good change regardless:

'assumes' in what way? If concrt140.dll uses msvcp140.dll internally but no STL objects are passed from user code (built with libc++) to ppl functions to concrt140.dll, things should be fine for example.

This didn't work out, we'll just skip the tests for MSVC++ for now.

We (MSVC++) could just fix pplwin.h to not have this assumption, since clang doesn't need the workaround that forces us to reinterperet_cast void*, but that fix probably wouldn't make it shipped until VS 2019 Update 1 at the earliest (I'd have to go through ship room) and Update 2 more likely.

Attempting to use ConcRT powered machinery (like the Concurrency:: primitives in this file) with a different STL like libc++ is likely to be non-functional, since that calls into concrt140.dll which assumes MSVC++'s STL. There's also an issue here that this code is using std::mutex with ConcRT powered machinery like Concurrency::parallel_for_each, which is not supported and is likely to result in deadlocks. ConcRT assumes that everything you give to it is non-blocking, so you can't use Concurrency::parallel_for_each with platform synchronization primitives. (This is the biggest reason we, MSVC++, are moving away from ConcRT as a technoligy and did not use it to power our parallel algorithms implementation)

This broke all the build bots, so I reverted the SVN change. I thought the nodiscard-ness was tested elsewhere but I'm not sure about interaction between this and -faligned-allocation.

As an FYI, I committed this to subversion as r356632 since I haven't figured out the new git world order yet.....

Thank you! :D

Actually this doesn't work -- because we /FI this thing, this file is included before the test gets to say _LIBCPP_ENABLE_DEPRECATION_WARNINGS.

Also fixed cstdlib tests.

Committed r344821

Committed r344820

Remove changes to detach tests.

In D50549#1194852, @EricWF wrote:

We should add some TSAN folks to this review, since I think these are also TSAN false negatives; perhaps correctly, perhaps not.

Adding ldionne as suggested by Eric.

Adding ldionne as suggested by Eric.

Committed r339214

Committed r339213

Committed r339212

Remove comma.

Committed r339209

In D49774#1175650, @ldionne wrote:

In D49774#1175543, @BillyONeal wrote:

In D49774#1175131, @mclow.lists wrote:

Another problem (that Eric and I discussed last night) is that filesystem is part of C++17, and file_clock is C++20. So we need a solution for C++17 as well.

It seems like we need to fix C++20 to allow that to be a typedef to a type in std::filesystem or that will be ABI breaking for MSVC++. IMO we should fix the spec to allow that rather than make libc++ jump through insane hoops.

We could also just provide file_clock "early" in C++17. Strictly speaking that may make our implementation non-conforming, but IDK how big of a deal this would be?

In D49774#1175131, @mclow.lists wrote:

Another problem (that Eric and I discussed last night) is that filesystem is part of C++17, and file_clock is C++20. So we need a solution for C++17 as well.

Why did tests for this this go into std? [reentrancy]/1 says this isn't required to work. Moreover, assignments in the dtor like this *can't* work in the general case because they would try to overwrite the SSO space. e.g. what do you expect this to do?