Page MenuHomePhabricator

[libcxx][modularisation] completes <memory> modularisation
AbandonedPublic

Authored by cjdb on Dec 3 2021, 11:34 AM.

Details

Reviewers
ldionne
Quuxplusone
Mordante
Group Reviewers
Restricted Project

Diff Detail

Event Timeline

cjdb created this revision.Dec 3 2021, 11:34 AM
cjdb requested review of this revision.Dec 3 2021, 11:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 3 2021, 11:34 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
cjdb updated this revision to Diff 391690.Dec 3 2021, 11:38 AM

removes bogus header include

libcxx/include/__memory/temp_value.h
45

I believe this was indented in the old code. I've pulled it back to the start of the line in this patch, but no other source changes have been intentionally applied.

Quuxplusone accepted this revision as: Quuxplusone.Dec 4 2021, 12:34 PM
Quuxplusone added a subscriber: Quuxplusone.

LGTM % comments!

libcxx/include/__vector/construct.h
1

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?

libcxx/include/memory
679

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.)

704

This should move to <__memory/align.h>. It's just a one-liner, but it lets us make <memory> a true top-level header with no extraneous bits. (And no libc++ header will ever need to resort to #include <memory> for any reason except backward compatibility.)

(You'll also get rid of _LIBCPP_BEGIN_NAMESPACE_STD / _LIBCPP_END_NAMESPACE_STD at that point.)

Mordante accepted this revision as: Mordante.Dec 5 2021, 5:18 AM
Mordante added a subscriber: Mordante.

In general LGTM modulo some minor issues. I'll omit the libc++ approval for now, so we have another look after all issues have been addressed.

libcxx/include/__functional/builtin_new_allocator.h
18
libcxx/include/__memory/swap_allocator.h
18
libcxx/include/__vector/construct.h
1

I agree. Alternatively how about splitting this in multiple files?

Quuxplusone added inline comments.Dec 5 2021, 7:25 AM
libcxx/include/__vector/construct.h
1

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/.)

ldionne added inline comments.Dec 6 2021, 7:42 AM
libcxx/include/__vector/construct.h
1

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.

libcxx/include/memory
679

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/.

cjdb added inline comments.Dec 6 2021, 3:18 PM
libcxx/include/memory
679

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>.

ldionne added inline comments.Dec 7 2021, 5:22 AM
libcxx/include/memory
679

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>.

cjdb added inline comments.Dec 7 2021, 2:10 PM
libcxx/include/memory
679

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.

ldionne added inline comments.Dec 7 2021, 3:18 PM
libcxx/include/memory
679

Without context on the discussion (which I don't recall), I can't comment further.

Agreed, but there's dozens of modularization patches with hundreds of comments on them in the aggregate, and Phab doesn't have a good search, so I won't bother. Let's pretend that the discussion never happened -- I don't think it has an impact on the point I'm trying to make.

Edit: I just found https://reviews.llvm.org/D107785#inline-1026088. It does provide some context but it doesn't involve you, so I was wrong, and it doesn't look like it was officially written in stone. However, see below.

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

Testing implementation details is often useful to increase our testing coverage when it's really difficult to black-box test something using only the standard API. We have some of it inside libcxx/test/libcxx, for example for testing helpers of <functional>. When that happens, we need a way to include the implementation detail header from a test file, and we can't use __memory/construct.h (for example) since that is a private submodule. IIRC, that is why we've been consistently including those implementation detail headers in the main header, so we should do it (if only) for consistency.

cjdb abandoned this revision.Tue, Jan 18, 11:54 AM