This is an archive of the discontinued LLVM Phabricator instance.

[libcxx][test] view_interface need not derive from view_base
ClosedPublic

Authored by CaseyCarter on Jan 18 2022, 2:26 PM.

Details

Summary

... after LWG-3549.

Diff Detail

Event Timeline

CaseyCarter requested review of this revision.Jan 18 2022, 2:26 PM
CaseyCarter created this revision.
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptJan 18 2022, 2:26 PM
Quuxplusone added inline comments.
libcxx/test/std/ranges/range.utility/view.interface/view.interface.pass.cpp
34–35

This is late-breaking C++20, right?
If so, just delete this line (and the following blank line): there's no point testing something that we (1) shouldn't do and (2) presumably plan to change.

Review comment.

CaseyCarter marked an inline comment as done.Jan 18 2022, 5:06 PM
Mordante accepted this revision as: Mordante.Jan 19 2022, 9:24 AM
Mordante added a subscriber: Mordante.

LGTM!

jloser added a subscriber: jloser.Jan 19 2022, 10:24 AM
jloser added inline comments.
libcxx/test/std/ranges/range.utility/view.interface/view.interface.pass.cpp
34–35

I'd prefer one of two different options:

  1. Make this LIBCPP_STATIC_ASSERT as the current view_interface class template does indeed inherit from view_base.
  2. Implement LWG3549 and make it so that view_interface class template no longer derives from view_base and then kill this line in the test as you've done.

Any appetite for implementing LWG3549? If not, I volunteer.

Quuxplusone accepted this revision.Jan 19 2022, 11:04 AM
Quuxplusone added inline comments.
libcxx/test/std/ranges/range.utility/view.interface/view.interface.pass.cpp
34–35

Any appetite for implementing LWG3549? If not, I volunteer.

I suspect I speak for us all when I say: go for it! :)
(I still don't think that ought to block Casey's landing this one-line PR, though, so I'm being the second libc++ accepter.)

This revision is now accepted and ready to land.Jan 19 2022, 11:04 AM
CaseyCarter added inline comments.Jan 19 2022, 11:09 AM
libcxx/test/std/ranges/range.utility/view.interface/view.interface.pass.cpp
34–35

I'm not interested in implementing LWG3549 (for a second time =)), I'm only motivated to have the test not apply non-standard requirements when stdlib=msvc. How about if we split the work: I'll remove this test line, and you can implement LWG3549?

Mordante added inline comments.Jan 20 2022, 9:56 AM
libcxx/test/std/ranges/range.utility/view.interface/view.interface.pass.cpp
34–35

It would be great when you implement LWG3549 @jloser!
I agree LWG3549 shouldn't block landing this commit.

jloser added inline comments.Jan 20 2022, 12:14 PM
libcxx/test/std/ranges/range.utility/view.interface/view.interface.pass.cpp
34–35

@CaseyCarter seems reasonable! https://reviews.llvm.org/D117714 is almost done for implementing LWG3539.