Page MenuHomePhabricator

[libc++] Implement P1169R4 (static operator())
Needs ReviewPublic

Authored by philnik on Oct 1 2022, 12:27 PM.

Details

Reviewers
ldionne
Mordante
var-const
huixie90
EricWF
Group Reviewers
Restricted Project

Diff Detail

Unit TestsFailed

TimeTest
900 mslibcxx CI C++2b > llvm-libc++-shared-cfg-in.std/utilities/function_objects/func_wrap/func_wrap_func/func_wrap_func_con::ctad.static.compile.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/clang++-16 /home/libcxx-builder/.buildkite-agent/builds/ecbe5da2461d-1/llvm-project/libcxx-ci/libcxx/test/std/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/ctad.static.compile.pass.cpp --target=x86_64-unknown-linux-gnu -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/ecbe5da2461d-1/llvm-project/libcxx-ci/build/generic-cxx2b/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/ecbe5da2461d-1/llvm-project/libcxx-ci/build/generic-cxx2b/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/ecbe5da2461d-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -Werror -Wall -Wctad-maybe-unsupported -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_ENABLE_EXPERIMENTAL -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -fsyntax-only
80 mslibcxx CI Modular build > llvm-libc++-shared-cfg-in.std/utilities/function_objects/func_wrap/func_wrap_func/func_wrap_func_con::ctad.static.compile.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/clang++-16 /home/libcxx-builder/.buildkite-agent/builds/aa423855f398-1/llvm-project/libcxx-ci/libcxx/test/std/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/ctad.static.compile.pass.cpp --target=x86_64-unknown-linux-gnu -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/aa423855f398-1/llvm-project/libcxx-ci/build/generic-modules/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/aa423855f398-1/llvm-project/libcxx-ci/build/generic-modules/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/aa423855f398-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -fmodules -fcxx-modules -Werror -Wall -Wctad-maybe-unsupported -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_ENABLE_EXPERIMENTAL -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -fsyntax-only

Event Timeline

philnik created this revision.Oct 1 2022, 12:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 1 2022, 12:27 PM
philnik requested review of this revision.Oct 1 2022, 12:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 1 2022, 12:27 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
EricWF requested changes to this revision.Oct 1 2022, 7:37 PM
EricWF added a subscriber: EricWF.

this needs real tests. compile only tests are not sufficient

This revision now requires changes to proceed.Oct 1 2022, 7:37 PM

this needs real tests. compile only tests are not sufficient

Why? CTAD is a compile-time feature. There is no runtime to test.

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.

this needs real tests. compile only tests are not sufficient

Why? CTAD is a compile-time feature. There is no runtime to test.

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?

ldionne requested changes to this revision.Oct 4 2022, 10:22 AM

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?

philnik updated this revision to Diff 465735.Oct 6 2022, 7:55 AM
  • Address comments

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.

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.

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.

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.

ldionne accepted this revision.Oct 6 2022, 8:41 AM

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.

19

Let's mark this as UNSUPPORTED: clang-14, clang-15, gcc-12 instead.

21

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.

huixie90 accepted this revision.Oct 8 2022, 7:38 AM
huixie90 added inline comments.
libcxx/include/__functional/function.h
1067–1068

Should this be C++23?

libcxx/include/future
2053

Should the new deduction guide be C++23?

philnik updated this revision to Diff 470297.Oct 24 2022, 2:57 PM
philnik marked 6 inline comments as done.
  • Rebased
  • Address comments
  • Try to fix CI
libcxx/include/__functional/function.h
1067–1068

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
19

I'm XFAILing it to ensure we test it on all compilers that support static operator().

21

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.