[libcxx-commits] [PATCH] D115059: [libcxx][modularisation] completes <memory> modularisation

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/__vector/construct.h:1
+// -*- C++ -*-
+//===----------------------------------------------------------------------===//
----------------
Quuxplusone wrote:
> Mordante wrote:
> > Quuxplusone wrote:
> > > I'm not thrilled by the name of this file, although I can't come up with anything super better, so the current name is not unacceptable AFAIC. //Possibly// `<__vector/construct_range.h>` or `<__vector/construct_helpers.h>` would be better?
> > I agree. Alternatively how about splitting this in multiple files?
> FWIW, I vote for not splitting this any //further//: all the functionality in here is tightly related. (But I see the counterargument, that the tight relation would still be implied by the fact that everything's under `__vector/`.)
I'm fine with whatever name -- In the long term, I'd actually try to generalize those algorithms to make them usable not only in `std::vector`, but elsewhere too.


================
Comment at: libcxx/include/memory:679
 #include <__memory/unique_ptr.h>
 #include <__memory/uses_allocator.h>
 #include <compare>
----------------
Quuxplusone wrote:
> I think we still haven't made a consistent decision about whether `<foo>` should include //all// `<__foo/bar.h>` subheaders (like the module does), or just the "public-relevant" ones. However, for consistency with how //this specific file// currently includes `<__memory/allocation_guard.h>`, please `#include` all your new headers in here too.
> 
> ...Except for `<__vector/construct.h>`, I suppose, since that's now part of `<vector>`. (It's basically as if we cut-and-pasted those components from here to there, in addition to the NFC parts of this patch. I think that's totally fine, because we're within our rights to shuffle around internal implementation details however often we like.)
Yeah, let's include all of the `__memory/foo.h` headers since that's what we've been doing. Agreed on not including `__vector/construct.h`, since it is not part of `__memory/`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115059