[libcxx-commits] [PATCH] D123600: [libc++][ranges] Implement `views::take`.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

This still LGTM, but there are a few comments from other reviewers that need addressing.



================
Comment at: libcxx/include/__fwd/string_view.h:1
+// -*- C++ -*-
+//===---------------------------------------------------------------------===//
----------------
philnik wrote:
> var-const wrote:
> > huixie90 wrote:
> > > philnik wrote:
> > > > huixie90 wrote:
> > > > > ldionne wrote:
> > > > > > philnik wrote:
> > > > > > > `__string_view/fwd.h` would make more sense I think. Same for `span`.
> > > > > > FWIW I don't really have a strong opinion. The only downside to this is that we might end up with a lot of one-header subdirectories if we start adding forward declarations to more classes, and putting everything under `__fwd/` would avoid that.
> > > > > I am in favour of fwd folder. I don’t like to have too many files with the exact same name fwd.hpp 
> > > > I don't like having a `__fwd` subdirectory because that would mean we have a single header spread across multiple subdirecories. That makes it a lot harder to see what is part of a header.
> > > I think having a single fwd folder whose subdirectories hierarchies follow the non-fwd hierarchies is pretty neat. You can easily find the forward declarations by adding a fwd at the beginning. One example code base is Boost.Hana 
> > I'm slightly in favor of having all the forward declarations under a single folder because it would make it easier to see what can and cannot be forward-declared. While having contents of a single header spread across several directories in general would be bad, I think splitting out forward declarations following a consistent pattern shouldn't cause any issues. @philnik How strongly do you feel about this?
> The reason you want to forward declare the things here is to avoid including all of the header and having longer compile times, right? If there is another technical reason for that, please tell me! So there shouldn't be any reason for new headers to have them in 2-3 years (hopefully).  I don't think there will be a lot of new forward declared headers because of this, so I think having forward declarations in another directory is quite surprising.
> So yeah, I do feel quite strongly about this.
The main reason for having forward declarations is to avoid circular dependencies. For example, `<span>` includes some headers from `<__ranges/...>`, and it's possible that some of them also require including `<span>`. Having an easy way to add forward declarations is useful for that.

I don't want us to block this patch on such a simple issue. Let's add forward declarations in `__fwd/FOO.h` from here on, and we can always change that (or even try to remove as many forward declarations as possible) in the future, if there is a good reason to do so.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123600