This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Explicitly reject URNG types with signed result_types.
ClosedPublic

Authored by Quuxplusone on Feb 27 2022, 9:01 AM.

Details

Summary

Fixes https://github.com/llvm/llvm-project/issues/48965

At first I tried making one big .verify.cpp test for all the distributions, but it turned out that that had a problem: Some distributions depend on other distributions. So instantiating lognormal_distribution::operator() will produce an error inside normal_distribution::operator() as well as its "expected" error. This means that one distribution can trigger up to 7(!) static_asserts. And also, once normal_distribution's assert has been triggered transitively by lognormal_distribution, well, now it's been instantiated, so it will not be triggered a second time when we get down to the place in the test where we actually want to test normal_distribution.

So, I've somewhat guiltily gone ahead and added a bad_engine.verify.cpp for every distribution individually. I placed it in test/std/ because my impression is that .verify.cpp tests are run only on Clang+libc++ anyway; this isn't testing a libc++ extension per se, just a libc++-specific diagnostic for standard UB; and I didn't want to re-create the whole deeply nested directory structure in test/libcxx/ just for this.

Diff Detail

Event Timeline

Quuxplusone requested review of this revision.Feb 27 2022, 9:01 AM
Quuxplusone created this revision.
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptFeb 27 2022, 9:01 AM

Rebase on main.

ldionne added inline comments.Mar 1 2022, 10:34 AM
libcxx/test/std/numerics/rand/rand.dis/rand.dist.bern/rand.dist.bern.bernoulli/bad_engine.verify.cpp
10

You should mark those as REQUIRES: libc++ since they are not under libcxx/test/libcxx.

ldionne accepted this revision.Mar 1 2022, 10:34 AM

Apart from that and the failing CI, this LGTM.

This revision is now accepted and ready to land.Mar 1 2022, 10:34 AM

Move the new tests to libcxx/test/libcxx/, rather than make them the only ones that require libc++. (Louis tells me that .verify.cpp tests run only with Clang, because they rely on clang -verify; but they don't necessarily run only with libc++.)

Herald added a project: Restricted Project. · View Herald TranscriptMar 1 2022, 6:11 PM