This is an archive of the discontinued LLVM Phabricator instance.

[libc++] [test] Remove `UNSUPPORTED: stdlib=msvc` from lock.pass.cpp
ClosedPublic

Authored by Quuxplusone on Jan 5 2022, 4:34 PM.

Details

Summary

@CaseyCarter says, "We run std/thread/thread.mutex/thread.lock.algorithm/lock.pass.cpp on every CI and it works fine with MSVC STL. I'm guessing this annotation predates @BillyONeal's rewrite of std::lock in 2017-ish."

Diff Detail

Event Timeline

Quuxplusone requested review of this revision.Jan 5 2022, 4:34 PM
Quuxplusone created this revision.
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptJan 5 2022, 4:34 PM
philnik accepted this revision.Jan 5 2022, 4:40 PM

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.

Mordante accepted this revision as: Mordante.Jan 6 2022, 9:00 AM
Mordante added a subscriber: Mordante.

LGTM

Quuxplusone added inline comments.Jan 6 2022, 9:27 AM
libcxx/test/std/thread/thread.mutex/thread.lock.algorithm/lock.pass.cpp
109–113

@BillyONeal: Oh, I think I see what you mean. This test isn't conforming, right? Because here type L1 has a try_lock method that literally never succeeds. So this test might reasonably spin forever, spinning trying to lock L1 l1 via try_lock and never succeeding. The only reason it "works" with libc++ (and presumably now MSVC) is that when l1.try_lock() fails, we unlock everything and start over with a blocking l1.lock() (which happens to succeed).
I'll adjust the comment, but also, gross. :P

Update the comment.

It feels like this test maybe shouldn't even be in libcxx/test/std/, but it's also our only test coverage for std::lock at all, and splitting it up into "conforming" and "non-conforming" pieces seems like more than I signed up for. So I'm going to resist scope creep and land this once CI is green. If someone else wants to tackle the "write good std::lock tests" task, that'd be cool.

This revision was not accepted when it landed; it landed in state Needs Review.Jan 8 2022, 1:03 PM
This revision was automatically updated to reflect the committed changes.