[libcxx-commits] [PATCH] D106107: [libc++][modularisation] Split <compare> into internal headers

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

Quuxplusone accepted this revision.
Quuxplusone added a comment.
This revision is now accepted and ready to land.

LGTM mod nits.



================
Comment at: libcxx/include/CMakeLists.txt:100-102
   __config
+  __compare/common_comparison_category.h
+  __compare/ordering.h
----------------
a-z plz


================
Comment at: libcxx/include/__compare/common_comparison_category.h:9-10
+
+#ifndef _LIBCPP___COMMON_COMPARISON_CATEGORY_H
+#define _LIBCPP___COMMON_COMPARISON_CATEGORY_H
+
----------------
I think our style would make this `_LIBCPP___COMPARE_COMMON_COMPARISON_CATEGORY_H` — please check some other nested-directory files, and then change it here if I'm right.


================
Comment at: libcxx/include/__compare/common_comparison_category.h:12-13
+
+#include <__config>
+#include <__compare/ordering.h>
+#include <type_traits>
----------------
a-z plz


================
Comment at: libcxx/include/__compare/common_comparison_category.h:29
+
+enum _ClassifyCompCategory : unsigned{
+  _None,
----------------
If you want, a drive-by whitespace fix to `unsigned {` would be nice.


================
Comment at: libcxx/include/compare:133-134
 
+_LIBCPP_PUSH_MACROS
+#include <__undef_macros>
+
----------------
Is this needed now for some reason? (Historically we seem to have put `__undef_macros` only in files that actually use `min` or `max` as identifiers. If something breaks without these lines, it'd be nice to say in your commit message what it is; and if nothing breaks without these lines, then //arguably// they shouldn't be here. Although I'm not inclined to //actually// object.)


================
Comment at: libcxx/include/module.modulemap:359-360
+    module __compare {
+      module ordering                   { private header "__compare/ordering.h"                   }
+      module common_comparison_category { private header "__compare/common_comparison_category.h" }
+    }
----------------
a-z plz


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

https://reviews.llvm.org/D106107