This is an archive of the discontinued LLVM Phabricator instance.

[libcxx][functional][modular] splices <functional> into modular headers
ClosedPublic

Authored by ldionne on Jun 25 2021, 12:09 PM.

Diff Detail

Event Timeline

cjdb created this revision.Jun 25 2021, 12:09 PM
cjdb requested review of this revision.Jun 25 2021, 12:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 25 2021, 12:09 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne requested changes to this revision.Jun 28 2021, 8:54 AM

Some comments, but I like the direction!

libcxx/include/CMakeLists.txt
105–148

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

111–114

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.

116–122

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.

This revision now requires changes to proceed.Jun 28 2021, 8:54 AM
Quuxplusone added inline comments.
libcxx/include/CMakeLists.txt
111–114

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).
For example, you'll be able to diff bit_and.h bit_xor.h in order to detect accidental differences between the implementations. With those classes both in the same header, it's much harder to detect accidental differences. (Of course, with those classes in different headers, it's much easier to introduce accidental differences, by forgetting to edit one of the million headers. But we've already committed to that disadvantage.)

116–122

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.
For example,

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

cjdb added inline comments.Jun 28 2021, 10:11 AM
libcxx/include/CMakeLists.txt
105–148

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

111–114

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.

ldionne added inline comments.Jun 28 2021, 2:06 PM
libcxx/include/CMakeLists.txt
105–148

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.

111–114

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.

cjdb added inline comments.Jun 29 2021, 8:37 AM
libcxx/include/CMakeLists.txt
105–148

Let's do this in a separate commit. Feel free to do it: just lmk so we don't duplicate work like in <iterator> 🙃

111–114

I also understand @cjdb's point about name leakage, however name leakage isn't really an issue -- those are not macros.

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.

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.

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.

ldionne added inline comments.Jun 29 2021, 9:05 AM
libcxx/include/CMakeLists.txt
105–148
111–114

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.

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
3

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.

cjdb added inline comments.Jun 29 2021, 11:26 AM
libcxx/include/CMakeLists.txt
105–148

Excellent, thank you!

111–114

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.

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

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.

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
111–114

We keep the 19 headers, and I add grouped ones for simplicity too

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

cjdb updated this revision to Diff 355433.Jun 29 2021, 8:30 PM
cjdb marked 6 inline comments as done.
  • 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.

ldionne added inline comments.Jun 30 2021, 2:42 PM
libcxx/include/__functional/binary_negate.h
10

Those include guards are now wrong!

cjdb updated this revision to Diff 355770.EditedJun 30 2021, 9:22 PM

* replaces specialisations with _If
* adds tests
* moves borrowed_subrange_t into __range/subrange.h (ATTN @ldionne)

Pushed wrong worktree.

cjdb updated this revision to Diff 355777.Jun 30 2021, 10:27 PM

moves function objects into a single header

cjdb updated this revision to Diff 355788.Jul 1 2021, 12:16 AM

forgot to add the new header

ldionne commandeered this revision.Jul 1 2021, 6:48 AM
ldionne edited reviewers, added: cjdb; removed: ldionne.

Commandeering to fix the few last issues and get a CI build going before Chris is up!

libcxx/include/CMakeLists.txt
132–137

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.

ldionne marked 3 inline comments as done.Jul 1 2021, 7:06 AM
ldionne updated this revision to Diff 355867.Jul 1 2021, 7:06 AM

Apply final comments and get a CI build going

ldionne accepted this revision.Jul 1 2021, 11:01 AM

CI passing, shipping this now. Thanks @cjdb for doing this work despite our differing views on how to split the functional operations.

This revision is now accepted and ready to land.Jul 1 2021, 11:01 AM
libcxx/include/experimental/functional