Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
Are the packaged_task changes intentionally missing? I haven't found any deduction guides in source for packaged_task so maybe they're implicit somehow and I miss it.
Because people run their code sometimes. And we should too.
And because writing holistic tests catches more than just bugs in the change.
They can catch compiler frontend and backend bugs, latent bugs in existing code, etc.
Because shipping a bug in libc++ is such a high cost, it's worth it to test more than the bare minimum.
Yes, it deduces to the correct type. But does it correctly call the constructor and store the pointer? It should, but does it?
More generally speaking, I think Eric's point is that we should also add tests that ensure that std::function works with a static operator(), regardless of CTAD. I agree with that, however I can see why that could be seen as separate from this patch.
But yes, I think we should definitely have runtime tests to check the interaction of static operator() and std::function.
| libcxx/test/std/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/ctad.static.compile.pass.cpp | ||
|---|---|---|
| 11 | Why do we support this pre-C++23? | |
No, I just missed them somehow. It also seems that we don't have CTAD implemented for packaged_task yet, so I did it here.
I would consider this a separate issue, since there are probably other areas where we want to add static operator() tests.
| libcxx/test/std/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/ctad.static.compile.pass.cpp | ||
|---|---|---|
| 11 | Because my plan is to use static operator() for the implementation of the ranges algorithms, since it improves code gen. | |
This LGTM with my comments.
| libcxx/test/std/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/ctad.static.compile.pass.cpp | ||
|---|---|---|
| 11 | Range algorithms can't be passed to std::function if we follow the letter of the standard. I don't think we should attempt to support this before C++23. If we do, then we are almost certainly missing similar things elsewhere in the library, which would make our support for that spotty anyway. Let's just implement this in C++23, as specified, and we can make our ranges algorithms' operator() static, which shouldn't be detectable. While people can technically detect it by looking at the type of e.g. std::ranges::copy::operator(), they are not allowed to do that in the first place, which makes this undetectable. | |
| 18 | Let's mark this as UNSUPPORTED: clang-14, clang-15, gcc-12 instead. | |
| 20 | Please add a few more cases: 0 arguments, 1 argument, variadic arguments, default argument (why not?). Also, let's run these tests (not compile only), unless you sign up to address the more general issue of testing the integration of static operator() in std::function (and probably other places). If you don't sign up for that, then let's run these tests cause that's going to be better than nothing. | |
- Rebased
- Address comments
- Try to fix CI
| libcxx/include/__functional/function.h | ||
|---|---|---|
| 1070–1071 | No, this has been added in C++17. | |
| libcxx/include/future | ||
| 2053 | No, this is from LWG3117, which IIUC should be applied retroactively to C++17. | |
| libcxx/test/std/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/ctad.static.compile.pass.cpp | ||
| 18 | I'm XFAILing it to ensure we test it on all compilers that support static operator(). | |
| 20 | I'd rather do it not at all or properly, than half-assed, so I guess I'm signing up for it. Otherwise it probably happens in 5 years. | |
Should this be C++23?