[libcxx-commits] [PATCH] D134952: [libc++][ranges]implement `std::views::take_while`

[Hui via Phabricator via libcxx-commits]

huixie90 added inline comments.


================
Comment at: libcxx/docs/Status/Cxx2bIssues.csv:1
 "Issue #","Issue Name","Meeting","Status","First released version","Labels"
 "`2839 <https://wg21.link/LWG2839>`__","Self-move-assignment of library types, again","November 2020","|Nothing To Do|",""
----------------
philnik wrote:
> Could you maybe do a survey of what we have to still implement for P1035R7? It's one of the really large papers, so it would be nice to document what we have already implemented. Skimming through the paper it looks like we're only missing  `elements_view` and `take_while_view`.
I did a survey in this patch
https://reviews.llvm.org/D133317?vs=458020&id=458045

Basically there are 4 views left in the paper
`istream_view` (in progress)
`take_while_view`(in progress)
`drop_while_view`(to do)
`elements_view` (to do)

Louis pointed out that the remaining work in this paper isn't huge and we probably don't need to break it up in more csvs


================
Comment at: libcxx/include/__ranges/take_while_view.h:51
+  _LIBCPP_NO_UNIQUE_ADDRESS _View __base_ = _View();
+  _LIBCPP_NO_UNIQUE_ADDRESS __copyable_box<_Pred> __pred_;
+
----------------
philnik wrote:
> The standard calls this `movable-box`. Has the standard renamed it at some point?
`copyable-box` is replaced by `movable-box` in P2494R2, which we haven't implemented yet


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.take.while/sentinel/ctor.base.pass.cpp:70-74
+    int i     = 10;
+    int* iter = &i;
+    Sentinel s(Sent{0}, &pred);
+
+    [[maybe_unused]] bool b = iter == s;
----------------
philnik wrote:
> `i`, `iter` and `b` seem to do nothing here, or am I missing something?
I'd like to test that the constructor correctly initialise the member predicate pointer to the one passed in. The only observable way is to use `==`. Here I simply trigger the `==` and assert that the `pred` is called


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/sentinel/ctor.default.pass.cpp:1
 //===----------------------------------------------------------------------===//
 //
----------------
philnik wrote:
> Did you edit this file accidentally, so is Phabricator just showing a weird diff?
ops. I used clangd's rename in one test and it somehow renamed the class also in this file. (clangd is technically correct. It just wasn't aware that our tests are completely different programme. Otherwise these types defined in the tests would refer to the same symbol as all of them have external linkage)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134952/new/

https://reviews.llvm.org/D134952