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

[Louis Dionne via Phabricator via libcxx-commits]

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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104942