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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added inline comments.


================
Comment at: libcxx/include/memory:679
 #include <__memory/unique_ptr.h>
 #include <__memory/uses_allocator.h>
 #include <compare>
----------------
ldionne wrote:
> cjdb wrote:
> > ldionne wrote:
> > > 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/`.
> > There's precedent where we don't do this from `<functional>`. I'm also not in favour of exporting implementation details, which is what this would enable (i.e. I'd prefer to go and make a CL that deletes the detail headers in other user-facing headers).
> > 
> > A lot of the stuff in this CL would be better off in some `__container` directory anyway, since they're common to containers headers, rather than stuff that genuinely is in `<memory>`.
> I'm fairly sure we had a discussion where we all agreed on including everything blindly, and I even think you were pushing on doing that -- unless my memory is failing me. Concretely, I think we might need to export implementation details if we want to be able to test them, since we're not allowed to include implementation detail headers. So say we wanted to test `__memory/construct.h`, right now we need to include `<memory>` and rely on the fact that it includes `<__memory/construct.h>`.
Without context on the discussion (which I don't recall), I can't comment further.

I am against directly testing implementation details in general, and exposing them just for the sake of testing them is smelly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115059