[libcxx-commits] [PATCH] D122506: [libc++] Ensure that all public C++ headers include <__assert>

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

In general quite happy. I think we can do some more minor improvement.



================
Comment at: libcxx/include/ext/hash_map:207
 #include <__hash_table>
+#include <algorithm>
 #include <ext/__hash>
----------------
Just curious did this fail to compile?


================
Comment at: libcxx/include/filesystem:12
+
+
 /*
----------------
Please change to one blank line.


================
Comment at: libcxx/include/version:196
 
+#include <__assert> // all public C++ headers provide the assertion handler
 #include <__config>
----------------
I wonder when we include the `__assert` here, how do you feel about requiring `version` in every header instead?
When not, maybe extend the script to also always validate `version` is included? Looking at the number of proposals adding this header, soon every header will require them.


================
Comment at: libcxx/utils/generate_assertion_tests.py:10
+
+header_restrictions = {
+    "barrier": "!defined(_LIBCPP_HAS_NO_THREADS)",
----------------
This code feels duplicated. I don't care too strongly about it, but when we keep duplicating it more times we maybe should look at having one source of truth for these lists.


================
Comment at: libcxx/utils/generate_assertion_tests.py:20
+    "filesystem": "!defined(_LIBCPP_HAS_NO_FILESYSTEM_LIBRARY)",
+    "format": "!defined(_LIBCPP_HAS_NO_INCOMPLETE_FORMAT)",
+
----------------
I think this restriction isn't required it's safe to include this header when format is disabled.


================
Comment at: libcxx/utils/generate_assertion_tests.py:32
+    "ostream": "!defined(_LIBCPP_HAS_NO_LOCALIZATION)",
+    "ranges": "!defined(_LIBCPP_HAS_NO_INCOMPLETE_RANGES)",
+    "regex": "!defined(_LIBCPP_HAS_NO_LOCALIZATION)",
----------------
I think this restriction isn't required it's safe to include this header when ranges are disabled.


================
Comment at: libcxx/utils/generate_assertion_tests.py:44
+    "experimental/coroutine": "!defined(_LIBCPP_HAS_NO_EXPERIMENTAL_COROUTINES)",
+    "coroutine": "!defined(_LIBCPP_HAS_NO_CXX20_COROUTINES)",
+
----------------
I think this restriction isn't required it's safe to include this header when coroutines are disabled.


================
Comment at: libcxx/utils/generate_assertion_tests.py:56
+    "experimental/unordered_set": "__cplusplus >= 201103L",
+    "experimental/vector": "__cplusplus >= 201103L",
+}
----------------
Except for `experimental/regex` I don't see an `#error` in these headers. Do we need them in this list?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122506