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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik accepted this revision as: philnik.
philnik added a comment.

LGTM % nits.



================
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|",""
----------------
huixie90 wrote:
> 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
We don't need a csv, but marking the paper as in progress and adding a note that says that we have to implement `drop_while_view` and `elements_view` would be nice.


================
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;
----------------
huixie90 wrote:
> 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
The comparison should be `true`, right? Could you assert that?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134952