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

[Mark de Wever via Phabricator via libcxx-commits]

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

LGTM! I've some replies but they require no actions.



================
Comment at: libcxx/include/version:196
 
+#include <__assert> // all public C++ headers provide the assertion handler
 #include <__config>
----------------
ldionne wrote:
> 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.
Thanks for volunteering!


================
Comment at: libcxx/utils/generate_assertion_tests.py:10
+
+header_restrictions = {
+    "barrier": "!defined(_LIBCPP_HAS_NO_THREADS)",
----------------
ldionne wrote:
> 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.
That wasn't clear, but sounds good!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122506