[libcxx-commits] [PATCH] D104942: [libcxx][functional][modular] splices <functional> into modular headers

Louis Dionne via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Tue Jun 29 09:05:30 PDT 2021


ldionne added inline comments.


================
Comment at: libcxx/include/CMakeLists.txt:105-107
+  __functional/__perfect_forward.h
+  __functional/__is_transparent.h
+  __functional/__weak_result_type.h
----------------
cjdb wrote:
> ldionne wrote:
> > cjdb wrote:
> > > ldionne wrote:
> > > > 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).
> > > 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>`.
> > 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.
> Let's do this in a separate commit. Feel free to do it: just lmk so we don't duplicate work like in `<iterator>` 🙃 
https://reviews.llvm.org/rGd03aa7d6b66fd741db2d937c18a6c6675037b888


================
Comment at: libcxx/include/CMakeLists.txt:111-114
+  __functional/bit_and.h
+  __functional/bit_not.h
+  __functional/bit_or.h
+  __functional/bit_xor.h
----------------
cjdb wrote:
> ldionne wrote:
> > cjdb wrote:
> > > Quuxplusone wrote:
> > > > ldionne wrote:
> > > > > 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.
> > > > 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.)
> > > 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.
> > 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.
> >  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`.
> 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.



================
Comment at: libcxx/include/__functional/negate.h:2
+// -*- C++ -*-
+//===------------------------ functional ----------------------------------===//
+//
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104942



More information about the libcxx-commits mailing list