Details
- Reviewers
zoecarver Mordante cjdb ldionne - Group Reviewers
Restricted Project - Commits
- rG050b064f15ee: [libcxx][functional][modular] splices <functional> into modular headers
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Some comments, but I like the direction!
libcxx/include/CMakeLists.txt | ||
---|---|---|
107–127 | I would name those headers without leading underscores. Leading underscores are only useful to avoid conflicting with user-defined macros or user-defined filenames, but we already take our precautions here by using __functional. I think the intent here is to segregate implementation details from the public library API, however I don't think that adding two underscores here is the way to go. If we really wanted to do that, we could use something like detail/ instead (I'm not suggesting we do that at this point in time, only if it becomes helpful to do so). | |
113–116 | I would group all those operations (including the other functional operations like less) into a single header __functional/operations.h or something like that. They are all very similar, do not have major dependencies (beyond unary_function and binary_function), and we don't gain much from splitting them apart IMO. | |
118–124 | I don't think we want to move those to a deprecated subdirectory. Being deprecated is a property that depends on the standard version, and as such, it's not something that is inherent to the utility contained in the file. |
libcxx/include/CMakeLists.txt | ||
---|---|---|
113–116 | FWIW, I wasn't going to object to Chris's design here. ;) I personally wouldn't have bothered to split these up, but now that they are split up, I can see certain advantages (as well as disadvantages — but the disadvantages are common to this entire past couple months of refactoring; there's nothing uniquely bad about what's happening with these functional ops). | |
118–124 | Strongly agreed with @ldionne here. Also, three-level nesting is bad. | |
libcxx/include/iterator | ||
559 ↗ | (On Diff #354573) | I suggest (as a general rule) that removing header dependencies should be done in a separate commit, so that it's easier to revert if we need to. #include <iterator> int main() { typeid(42); } currently compiles with libc++, but if you remove __functional_base from iterator, then (I think) it stops compiling. |
libcxx/include/module.modulemap | ||
455–462 | On the one hand, this looks reasonable; on the other hand, I think we need a discussion about three-level nesting. And ideally a discussion about four-level nesting, before someone decides to do it. |
libcxx/include/CMakeLists.txt | ||
---|---|---|
107–127 | I can get behind a __header/detail approach. __functional/search.h "clashes" with __algorithm/search.h: std::search is in __algorithm, but __functional/search.h is an implementation detail used by __algorithm/search.h, __functional/default_searcher.h, and <regex>. | |
113–116 | Arthur's point is a good maintenance one (that I hadn't considered before). When designing this, I was going to settle on a smaller set of headers: arithmetic_ops.h, bitwise_ops.h, and comparison_ops.h, logical_ops.h. I decided to split it up even more granularly because I want to minimise name leakage. For example, <numeric> needs std::plus, std::minus, and std::multiplies, but it doesn't need anything else. Similarly, many algorithms only need std::equal_to or std::less, but nothing else. I'd like to minimise the surface area of what is leaked by necessity, which led to having singleton headers. | |
libcxx/include/iterator | ||
559 ↗ | (On Diff #354573) | Hmm, yes, good point (that commit is D104982). I'll track down the names from __functional_base and re-import them here. Thanks for flagging this. |
libcxx/include/module.modulemap | ||
455–462 | Pinged you on Discord. |
libcxx/include/CMakeLists.txt | ||
---|---|---|
107–127 | Actually, now that you bring this up, I don't see why __search needs to be in __functional/search.h. I think it should be in __algorithm/search.h directly, and its only caller (default_searcher) should use std::search directly, not std::__search. This can be done as a separate commit, let me know if you'd like me to do it. | |
113–116 | You make a point about maintenance, however putting them in the same header would also lend itself to improvements like defining them automatically with a macro. I also understand @cjdb's point about name leakage, however name leakage isn't really an issue -- those are not macros. If those had different sets of dependencies, then I wouldn't argue for this at all. However, they are all logically part of the same piece of functionality (providing a named version of various operators), so I think it makes more sense to group them. |
libcxx/include/CMakeLists.txt | ||
---|---|---|
107–127 | Let's do this in a separate commit. Feel free to do it: just lmk so we don't duplicate work like in <iterator> 🙃 | |
113–116 |
We're talking about different kinds of name leakage. I'm referring to the fact that anyone who includes __functional/operations.h implicitly exports all of the operations, even if they're not necessary. Hyrum's law indicates that <set> (for example) will be used to get function objects, because it can be used. <set> needs to export std::less for a working implementation of std::set, but it doesn't need to export std::greater or std::plus.
My argument above can also be made for <numeric>, where this is possibly worse than <set>, because that's where std::accumulate and std::inner_product* live. I wager that folks who understand how a fold operation can be the basis for a great many algorithms will reach for standard function objects more frequently than others. While it would be nice to make their lives easier and export them here, that exposes them to the sharp edge that std::equal_to isn't guaranteed to be provided by <numeric>, exposing their code to non-portability. *and std::reduce, and std::transform_reduce. |
libcxx/include/CMakeLists.txt | ||
---|---|---|
107–127 | ||
113–116 |
I agree that name leakage is bad, and that we can and should take reasonable steps to limit it. I'm personally still paying the price for <memory> once exposing the assert macro through an unfortunate include of <cassert>, which was removed over a year ago. However, I think we're giving name leakage more importance than it deserves in this instance. Those names are not used *that* often, and people who care that much about name leakage can/should use modules. I care more about grouping things together coherently when it makes sense and not having to include 19 headers individually than about name leakage, in this specific case. | |
libcxx/include/__functional/negate.h | ||
2 ↗ | (On Diff #354573) | Please remove the filename here and everywhere else. We shouldn't do that anymore, there's not any benefit AFAICT and we get it wrong when we copy/paste. |
libcxx/include/CMakeLists.txt | ||
---|---|---|
107–127 | Excellent, thank you! | |
113–116 |
I disagree on both counts. See my point regarding fold operations for the former (and also recognise that std::set<T, std::greater<>> is not an unreasonable thing to spell). For the latter: enabling modules might not always be possible. GCC doesn't support Clang modules as far as I'm aware, and I'm not exactly sure how many people even know about Clang modules, let alone know why they're useful, or how to use them (properly). Enabling modules requires convincing a project owner that it's worth the buy-in, and that's not always easy/possible. I'm also not sure what effect this will have on people who build everything from source and have upstream dependencies.
How about a compromise? We keep the 19 headers, and I add grouped ones for simplicity too. This means we can use individual headers when we only need a small few, and the grouped ones when we need to import pretty much everything. __functional/arithmetic_ops.h __functional/bitwise_ops.h __functional/comparison_ops.h __functional/logical_ops.h |
libcxx/include/CMakeLists.txt | ||
---|---|---|
113–116 |
IMO that would be the worst of all worlds. I'm relatively neutral on "1 header" versus "19 headers" versus "4 headers," but I certainly dislike "19 + 4 = 23 headers." |
- brings <__functional_base> back as a shell to preserve header inclusions in this commit
- removes deprecated directory
- removes underscores from implementation-detail header names
libcxx/include/iterator | ||
---|---|---|
559 ↗ | (On Diff #354573) | It was easier to bring <__functional_base> back as a zombie. That header can go for real in D104982. |
libcxx/include/module.modulemap | ||
455–462 | Ping again. |
libcxx/include/__functional/binary_negate.h | ||
---|---|---|
11 | Those include guards are now wrong! |
* replaces specialisations with _If
* adds tests
* moves borrowed_subrange_t into __range/subrange.h (ATTN @ldionne)
Pushed wrong worktree.
Commandeering to fix the few last issues and get a CI build going before Chris is up!
libcxx/include/CMakeLists.txt | ||
---|---|---|
126–131 | The same reasoning that we discussed applies for these operations - let's group them. Also, let's try to avoid 3 levels of nesting if we can. | |
libcxx/include/__functional/bind.h | ||
14–19 | Please take the habit of sorting includes. | |
libcxx/include/__functional_base | ||
16 | Oops, those haven't been updated. |
CI passing, shipping this now. Thanks @cjdb for doing this work despite our differing views on how to split the functional operations.
I would group all those operations (including the other functional operations like less) into a single header __functional/operations.h or something like that. They are all very similar, do not have major dependencies (beyond unary_function and binary_function), and we don't gain much from splitting them apart IMO.