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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/ext/hash_map:207
 #include <__hash_table>
+#include <algorithm>
 #include <ext/__hash>
----------------
Mordante wrote:
> Just curious did this fail to compile?
Yeah, this started failing to compile, I think it's because previously we never included `ext/hash_map` alone in a file, we always had other headers, and `<algorithm>` was probably always included before `ext/hash_map`.


================
Comment at: libcxx/include/version:196
 
+#include <__assert> // all public C++ headers provide the assertion handler
 #include <__config>
----------------
Mordante wrote:
> 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.
I'd be fine with that, although we most likely want to include `<version>` in all C++ headers only (not C compatibility headers, for the same reason as here). I volunteer to do it in a separate patch.


================
Comment at: libcxx/utils/generate_assertion_tests.py:10
+
+header_restrictions = {
+    "barrier": "!defined(_LIBCPP_HAS_NO_THREADS)",
----------------
Mordante wrote:
> 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.
I fully agree -- maybe it wasn't clear, but I have a follow-on patch locally that removes all the other header generation scripts.


================
Comment at: libcxx/utils/generate_assertion_tests.py:56
+    "experimental/unordered_set": "__cplusplus >= 201103L",
+    "experimental/vector": "__cplusplus >= 201103L",
+}
----------------
Mordante wrote:
> Except for `experimental/regex` I don't see an `#error` in these headers. Do we need them in this list?
We do, because they use C++03-unfriendly constructs in the headers. That's why they were guarded similarly in previous header-generation tests, too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122506