Details
- Reviewers
ldionne • Quuxplusone Mordante - Group Reviewers
Restricted Project
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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. |
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.) |
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? |
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/.) |
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/. |
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>. |
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>. |
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. |
libcxx/include/memory | ||
---|---|---|
679 |
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.
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. |