Page MenuHomePhabricator

[libcxx] Implement view.interface.
ClosedPublic

Authored by zoecarver on May 2 2021, 8:37 PM.

Details

Reviewers
ldionne
cjdb
EricWF
Quuxplusone
Group Reviewers
Restricted Project
Commits
rG5671ff20d92b: [libcxx] Implement view.interface.
Summary

This will unblock work on ranges::view. Based on D101396.

Refs http://eel.is/c++draft/view.interface.

Diff Detail

Unit TestsFailed

TimeTest
2,430 mslibcxx CI ASAN > libc++.std/experimental/language_support/support_coroutines/end_to_end::expected.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/clang++ /home/libcxx-builder/.buildkite-agent/builds/e08e383a5ae0-1/llvm-project/libcxx-ci/libcxx/test/std/experimental/language.support/support.coroutines/end.to.end/expected.pass.cpp -v --target=x86_64-unknown-linux-gnu -g -fno-omit-frame-pointer -fsanitize=address -include /home/libcxx-builder/.buildkite-agent/builds/e08e383a5ae0-1/llvm-project/libcxx-ci/libcxx/test/support/nasty_macros.h -nostdinc++ -I/home/libcxx-builder/.buildkite-agent/builds/e08e383a5ae0-1/llvm-project/libcxx-ci/build/generic-asan/include/c++/v1 -I/home/libcxx-builder/.buildkite-agent/builds/e08e383a5ae0-1/llvm-project/libcxx-ci/build/generic-asan/projects/libcxx/include/c++build -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -D__STDC_CONSTANT_MACROS -I/home/libcxx-builder/.buildkite-agent/builds/e08e383a5ae0-1/llvm-project/libcxx-ci/libcxx/test/support -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -std=c++2b -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-atomic-alignment -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -fmodules-cache-path=/home/libcxx-builder/.buildkite-agent/builds/e08e383a5ae0-1/llvm-project/libcxx-ci/build/generic-asan/projects/libcxx/test/std/experimental/language.support/support.coroutines/end.to.end/Output/expected.pass.cpp.dir/t.tmp/ModuleCache -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -fcoroutines-ts -lc++experimental -L/home/libcxx-builder/.buildkite-agent/builds/e08e383a5ae0-1/llvm-project/libcxx-ci/build/generic-asan/./lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/e08e383a5ae0-1/llvm-project/libcxx-ci/build/generic-asan/./lib -L/home/libcxx-builder/.buildkite-agent/builds/e08e383a5ae0-1/llvm-project/libcxx-ci/build/generic-asan/./lib…
2,100 mslibcxx CI TSAN > libc++.std/experimental/language_support/support_coroutines/end_to_end::expected.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/clang++ /home/libcxx-builder/.buildkite-agent/builds/cff98fecaa99-1/llvm-project/libcxx-ci/libcxx/test/std/experimental/language.support/support.coroutines/end.to.end/expected.pass.cpp -v --target=x86_64-unknown-linux-gnu -g -fno-omit-frame-pointer -fsanitize=thread -include /home/libcxx-builder/.buildkite-agent/builds/cff98fecaa99-1/llvm-project/libcxx-ci/libcxx/test/support/nasty_macros.h -nostdinc++ -I/home/libcxx-builder/.buildkite-agent/builds/cff98fecaa99-1/llvm-project/libcxx-ci/build/generic-tsan/include/c++/v1 -I/home/libcxx-builder/.buildkite-agent/builds/cff98fecaa99-1/llvm-project/libcxx-ci/build/generic-tsan/projects/libcxx/include/c++build -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -D__STDC_CONSTANT_MACROS -I/home/libcxx-builder/.buildkite-agent/builds/cff98fecaa99-1/llvm-project/libcxx-ci/libcxx/test/support -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -std=c++2b -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-atomic-alignment -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -fmodules-cache-path=/home/libcxx-builder/.buildkite-agent/builds/cff98fecaa99-1/llvm-project/libcxx-ci/build/generic-tsan/projects/libcxx/test/std/experimental/language.support/support.coroutines/end.to.end/Output/expected.pass.cpp.dir/t.tmp/ModuleCache -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -fcoroutines-ts -lc++experimental -L/home/libcxx-builder/.buildkite-agent/builds/cff98fecaa99-1/llvm-project/libcxx-ci/build/generic-tsan/./lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/cff98fecaa99-1/llvm-project/libcxx-ci/build/generic-tsan/./lib -L/home/libcxx-builder/.buildkite-agent/builds/cff98fecaa99-1/llvm-project/libcxx-ci/build/generic-tsan/./lib…
1,000 mslibcxx CI Windows (DLL) > libc++.std/ranges/range_utility/view_interface::view.interface.pass.cpp
Script: -- : 'COMPILED WITH'; 'C:/Program Files/LLVM/bin\clang++.EXE' C:\ws\w4\llvm-project\libcxx-ci\libcxx\test\std\ranges\range.utility\view.interface\view.interface.pass.cpp -v --target=x86_64-pc-windows-msvc -nostdinc++ -IC:/ws/w4/llvm-project/libcxx-ci/build/windows-dll/include/c++/v1 '-IC:/ws/w4/llvm-project/libcxx-ci/build/windows-dll\include\c++build' '-IC:/ws/w4/llvm-project/libcxx-ci/libcxx\test/support' -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_STDIO_ISO_WIDE_SPECIFIERS -DNOMINMAX -D_LIBCPP_HAS_NO_INT128 -std=c++2b -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-atomic-alignment -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -fmodules-cache-path=C:\ws\w4\llvm-project\libcxx-ci\build\windows-dll\test\std\ranges\range.utility\view.interface\Output\view.interface.pass.cpp.dir\t.tmp/ModuleCache -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -L 'C:\BuildTools\VC\Tools\MSVC\14.28.29910\ATLMFC\lib\x64' -L 'C:\BuildTools\VC\Tools\MSVC\14.28.29910\lib\x64' -L 'C:\Program Files (x86)\Windows Kits\NETFXSDK\4.8\lib\um\x64' -L 'C:\Program Files (x86)\Windows Kits\10\lib\10.0.19041.0\ucrt\x64' -L 'C:\Program Files (x86)\Windows Kits\10\lib\10.0.19041.0\um\x64' -LC:/ws/w4/llvm-project/libcxx-ci/build/windows-dll/lib -LC:/ws/w4/llvm-project/libcxx-ci/build/windows-dll/lib -nodefaultlibs -nostdlib -lc++ -lvcruntime -lucrt -lmsvcrt -lmsvcprt -loldnames -o C:\ws\w4\llvm-project\libcxx-ci\build\windows-dll\test\std\ranges\range.utility\view.interface\Output\view.interface.pass.cpp.dir\t.tmp.exe
1,180 mslibcxx CI Windows (Static) > libc++.std/ranges/range_utility/view_interface::view.interface.pass.cpp
Script: -- : 'COMPILED WITH'; 'C:/Program Files/LLVM/bin\clang++.EXE' C:\ws\w32-1\llvm-project\libcxx-ci\libcxx\test\std\ranges\range.utility\view.interface\view.interface.pass.cpp -v --target=x86_64-pc-windows-msvc -nostdinc++ -IC:/ws/w32-1/llvm-project/libcxx-ci/build/windows-static/include/c++/v1 '-IC:/ws/w32-1/llvm-project/libcxx-ci/build/windows-static\include\c++build' '-IC:/ws/w32-1/llvm-project/libcxx-ci/libcxx\test/support' -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_STDIO_ISO_WIDE_SPECIFIERS -DNOMINMAX -D_LIBCPP_HAS_NO_INT128 -std=c++2b -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-atomic-alignment -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -fmodules-cache-path=C:\ws\w32-1\llvm-project\libcxx-ci\build\windows-static\test\std\ranges\range.utility\view.interface\Output\view.interface.pass.cpp.dir\t.tmp/ModuleCache -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -llibc++experimental -L 'C:\BuildTools\VC\Tools\MSVC\14.28.29910\ATLMFC\lib\x64' -L 'C:\BuildTools\VC\Tools\MSVC\14.28.29910\lib\x64' -L 'C:\Program Files (x86)\Windows Kits\NETFXSDK\4.8\lib\um\x64' -L 'C:\Program Files (x86)\Windows Kits\10\lib\10.0.19041.0\ucrt\x64' -L 'C:\Program Files (x86)\Windows Kits\10\lib\10.0.19041.0\um\x64' -LC:/ws/w32-1/llvm-project/libcxx-ci/build/windows-static/lib -LC:/ws/w32-1/llvm-project/libcxx-ci/build/windows-static/lib -nodefaultlibs -nostdlib 'C:/ws/w32-1/llvm-project/libcxx-ci/build/windows-static/lib\libc++.lib' -lvcruntime -lucrt -lmsvcrt -lmsvcprt -loldnames -o C:\ws\w32-1\llvm-project\libcxx-ci\build\windows-static\test\std\ranges\range.utility\view.interface\Output\view.interface.pass.cpp.dir\t.tmp.exe

Event Timeline

zoecarver created this revision.May 2 2021, 8:37 PM
zoecarver requested review of this revision.May 2 2021, 8:37 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 2 2021, 8:37 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
zoecarver planned changes to this revision.May 2 2021, 8:41 PM

I wanted to get this review up before Monday so that other folks can start on their views work. I'm planning a few changes (mainly to add some tests) so this isn't quite ready for review yet.

libcxx/include/__ranges/view_interface.h
38

I will remove this once the patch lands and I rebase.

131

Add a message here.

150

Review note: this is used until ranges::prev is implemented.

zoecarver updated this revision to Diff 342305.May 2 2021, 8:41 PM
  • Fix spacing.
cjdb requested changes to this revision.May 2 2021, 9:30 PM
cjdb added a subscriber: tcanens.

A good start! I'd appreciate some libcxx tests confirming that _LIBCPP_ASSERT fires when a view is empty, and some noexcept tests.

libcxx/include/__ranges/view_interface.h
38

It doesn't look like the concept is used in your patch anyway?

46

Please make this a reference.

60

Oh, this issue. I remember when gcc -fconcepts had this problem back in the day :(

150

Thanks for highlighting this, was about to comment.

libcxx/test/std/ranges/range.utility/view.interface/view.interface.pass.cpp
35

Can we please get a pointer-to-member and one pointer-to-member function too?

49–52

Nit: consider making this a hidden friend.

103

Please either rename or document why this function exists.

112

Nit: fv or fr? (I'd personally go with single-letter names to avoid the mental "what does this letter correspond to?".)

156

I think view_interface::data (the real one) is a function template, so it'll prefer the existing member function. @tcanens how far from the mark am I?

This revision now requires changes to proceed.May 2 2021, 9:30 PM
tcanens added inline comments.May 2 2021, 9:41 PM
libcxx/include/__ranges/data.h
58 ↗(On Diff #342305)

missing !?

zoecarver added inline comments.May 3 2021, 9:44 AM
libcxx/include/__ranges/view_interface.h
46

In this case T = Derived so I don't think it matters. I've added a test where *this is of type move only rvalue.

60

:(

Also, I couldn't do __can_empty inline.

And if you don't have this, it fails in a very confusing way. Spent a lot of time figuring out why view_interface sort of thought _Derived was an incomplete type but could call members on the type. I finally figured it out when I discovered that it only appeared to be an incomplete type when evaluating the constraints. I suspect clang is simply forgetting a record = record->getDefinition() somewhere.

libcxx/test/std/ranges/range.utility/view.interface/view.interface.pass.cpp
49–52

Am I able to define a hidden friend out of line? I don't want to update forward_iterator (unless we agree that would be a good idea).

156

That would make sense, except that it's the child that's getting called, i.e, view_interface::data. Regardless of which one is a template, I thought it would always go for the parent's definition.

zoecarver updated this revision to Diff 342427.May 3 2021, 9:45 AM
  • Apply review comments and general cleanup.
cjdb added inline comments.May 3 2021, 12:48 PM
libcxx/test/std/ranges/range.utility/view.interface/view.interface.pass.cpp
49–52

Oh, I see, it's not for the above type. My mistake.

156

BTW, the same is true for SFINAE.

zoecarver planned changes to this revision.May 6 2021, 5:46 PM
zoecarver added inline comments.
libcxx/include/__ranges/view_interface.h
118

Remove all the nodiscards except for empty.

Quuxplusone added inline comments.May 6 2021, 6:14 PM
libcxx/include/__ranges/view_interface.h
118

It would be appropriate to mark them _LIBCPP_NODISCARD_EXT (with tests).

cjdb added inline comments.May 6 2021, 9:43 PM
libcxx/include/__ranges/view_interface.h
118

Remove all the nodiscards except for empty.

Please elaborate.

It would be appropriate to mark them _LIBCPP_NODISCARD_EXT (with tests).

Why is this different to [[nodiscard]] + disabling the warning?

zoecarver added inline comments.May 11 2021, 11:08 AM
libcxx/include/__ranges/view_interface.h
118

Please elaborate.

We've discussed elsewhere, I think libc++ is only going to apply nodiscard to methods where a user might think that a side effect is happening (such as .empty()).

  • Apply review comments.
zoecarver updated this revision to Diff 347804.May 25 2021, 3:25 PM

Rebase on main.

ldionne requested changes to this revision.May 31 2021, 2:46 PM

Generally LGTM, with some comments!

libcxx/include/CMakeLists.txt
43–46

Should be sorted!

libcxx/include/__ranges/view_interface.h
38

Can drop nodiscard.

55

Why is that necessary? Is that the workaround for gcc -fconcepts you're discussing in the comments below?

150

This can be changed now (and also in the noexcept(...) above).

libcxx/test/std/ranges/range.utility/view.interface/view.interface.pass.cpp
12

I don't think this is necessary anymore.

184

Can you elaborate on what this TODO means?

This revision now requires changes to proceed.May 31 2021, 2:46 PM
CaseyCarter added inline comments.Jun 1 2021, 6:56 AM
libcxx/include/__ranges/view_interface.h
49

You're missing that the implicit conversion to of this expression to bool could be potentially throwing. We use something like:

c++
template <class T>
void implicit_convert_to(type_identity_t<T>) noexcept;
/* ... */
  noexcept(noexcept(implicit_convert_to<bool>(ranges::begin(__derived()) == ranges::end(__derived()))))
55

This is necessary to workaround https://bugs.llvm.org/show_bug.cgi?id=44833.

zoecarver marked 8 inline comments as done.Jun 1 2021, 11:00 AM
zoecarver added inline comments.
libcxx/include/__ranges/view_interface.h
49

Done and added a regression test.

libcxx/test/std/ranges/range.utility/view.interface/view.interface.pass.cpp
184

No longer applies, removed.

(IIRC, that assert failed because it was using view_interface's implementation for data.)

zoecarver updated this revision to Diff 349025.Jun 1 2021, 11:01 AM
zoecarver marked 2 inline comments as done.

Apply review comments.

zoecarver updated this revision to Diff 349027.Jun 1 2021, 11:03 AM
  • Rebase.
  • -- -> prev (again).
ldionne accepted this revision.Jun 1 2021, 11:39 AM

LGTM, but please wait for CI to finish.

(Also, we just spoke offline and you mentioned you needed to update the Status paper and the synopsis - please do it before landing).

libcxx/include/__ranges/view_interface.h
55

Thanks!

This revision was not accepted when it landed; it landed in state Needs Review.Jun 1 2021, 12:35 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.