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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

Some comments, but I like the direction!



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


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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104942