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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone accepted this revision.
Quuxplusone added inline comments.


================
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; }
+};
----------------
huixie90 wrote:
> 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.)
> This time I removed unnamed namespace and hopefully that error will go.

Yup. :) Occam's Razor: don't do something unless you need to. Here, adding an extra `namespace {` (or `static`) resulted in the need to add an extra `{ return nullptr; }`. Removing the one lets you remove the other. And this explains why our tests generally //don't// make things internal-linkage (unless that's relevant to what's being tested!).


================
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()), 
----------------
huixie90 wrote:
> 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.
> I thought brace initialisation is recommended in almost all cases (except the case you don't want the std::initializer_list)

You basically //never// want the std::initializer_list (I mean, you want it only when you're initializing a container; and here `jv` is not a container). My common-sense guidelines are here:
https://quuxplusone.github.io/blog/2019/02/18/knightmare-of-initialization/#simple-guidelines-for-variable-i

- Use `=` whenever you can.
- Use initializer-list syntax `{}` only for element initializers (of containers and aggregates).
- Use function-call syntax `()` to call a constructor, viewed as an object-factory.

I'm pleased to say that libc++'s codebase mostly follows these guidelines by default — via convergent evolution, rather than any effort of my own.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116808