[libcxx-commits] [PATCH] D116808: fix __simple_view concept in std::ranges

[Hui via Phabricator via libcxx-commits]

huixie90 added a comment.

@Quuxplusone I applied the suggestions and see if it compiles fine.



================
Comment at: libcxx/test/std/ranges/range.adaptors/range.drop/begin.pass.cpp:103
+    int non_const_calls = 0, const_calls = 0;
+    std::ranges::drop_view dropView(NonSimpleView{{}, &non_const_calls, &const_calls}, 4);
+    assert(dropView.begin() == nullptr);
----------------
Quuxplusone wrote:
> (Aside: That extra `{}` is an initializer for the empty base class `view_base`? Yuck. But if the language requires it... okay.) 
Yes, it wouldn't compile without the extra `{}`


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.drop/begin.pass.cpp:104
+    std::ranges::drop_view dropView(NonSimpleView{{}, &non_const_calls, &const_calls}, 4);
+    assert(dropView.begin() == nullptr);
+    assert(std::as_const(dropView).begin() == nullptr);
----------------
Quuxplusone wrote:
> My proposed version ( https://reviews.llvm.org/D116808#3229481 ) used just a single `int calls` so that we could write `calls == 10`, `calls == 11` in one line, and I think it ends up being a bit cleaner. But this is OK. I'm just asking that we make sure to check the right number of calls after //each// call to `begin`.
yeah I think they are doing the same thing. I prefer separate variables. I have to look at the implementation to figure out what `calls==10` really means. will make sure after each call to begin, the numbers are checked


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/begin.pass.cpp:29-30
+struct SimpleParentView : std::ranges::view_base {
+  const ChildView* begin() const { return nullptr; }
+  const ChildView* end() const { return nullptr; }
+};
----------------
Quuxplusone wrote:
> Here and lines 23–25, if these functions are never called, please remove their definitions.
> ```
>   const ChildView* begin() const;
>   const ChildView* end() const;
> ```
> If they //are// called, and I just missed it, then of course this is fine.
> (And if they're //not// called, then the fact that I just spent a couple minutes //looking// for calls that don't exist should explain why I'm asking for the definitions to be removed!)
It was not called. It was my desperate attempt to workaround this ci failure
https://reviews.llvm.org/harbormaster/unit/view/1774321/

will remove  and try again.  This time I removed unnamed namespace and hopefully that error will go.  (btw, I think it is still a good idea to stop defining symbols in global namespace even though each TU has its own main function and not supposed to link against anything.)


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/begin.pass.cpp:109
+  {
+    std::ranges::join_view<SimpleParentView> jv{};
+    static_assert(std::same_as<decltype(jv.begin()), 
----------------
Quuxplusone wrote:
> Here and line 102: remove the `{}`.
removed. but is there a reason why this is recommended?   I thought brace initialisation is recommended in almost all cases (except the case you don't want the std::initializer_list). From a user point of view, if you don't know if a type is an aggregate like thing or it has proper default constructor, using `{}` will always initialise it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116808