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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added inline comments.


================
Comment at: libcxx/include/CMakeLists.txt:105-107
+  __functional/__perfect_forward.h
+  __functional/__is_transparent.h
+  __functional/__weak_result_type.h
----------------
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>`.


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


================
Comment at: libcxx/include/iterator:559
 #include <__debug>
-#include <__functional_base>
 #include <__iterator/advance.h>
----------------
Quuxplusone wrote:
> 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.
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.


================
Comment at: libcxx/include/module.modulemap:455-462
+      module ranges {
+        module equal_to      { header "__functional/ranges/equal_to.h"      }
+        module greater_equal { header "__functional/ranges/greater_equal.h" }
+        module greater       { header "__functional/ranges/greater.h"       }
+        module less_equal    { header "__functional/ranges/less_equal.h"    }
+        module less          { header "__functional/ranges/less.h"          }
+        module not_equal_to  { header "__functional/ranges/not_equal_to.h"  }
----------------
Quuxplusone wrote:
> 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.
Pinged you on Discord.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104942