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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


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


================
Comment at: libcxx/include/CMakeLists.txt:116-122
+  __functional/deprecated/binary_negate.h
+  __functional/deprecated/binder1st.h
+  __functional/deprecated/binder2nd.h
+  __functional/deprecated/mem_fun_ref.h
+  __functional/deprecated/pointer_to_binary_function.h
+  __functional/deprecated/pointer_to_unary_function.h
+  __functional/deprecated/unary_negate.h
----------------
ldionne wrote:
> 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.
Strongly agreed with @ldionne here. Also, three-level nesting is bad.


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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104942