[libcxx-commits] [PATCH] D118616: [libc++] Use -I instead of -isystem to include headers in the test suite

Tue Mar 1 11:33:07 PST 2022

Quuxplusone added inline comments.

================
Comment at: libcxx/include/ext/__hash:19-22
+_LIBCPP_DIAGNOSTIC_PUSH
+_LIBCPP_CLANG_DIAGNOSTIC_IGNORED("-Wprivate-header")
+#include <__string>
+_LIBCPP_DIAGNOSTIC_POP
----------------
EricWF wrote:
> philnik wrote:
> > ldionne wrote:
> > > philnik wrote:
> > > > Quuxplusone wrote:
> > > > > Here and below, this shouldn't be necessary. Remember the economist's $100 bill: if this were needed, why wouldn't we be doing it everywhere? (E.g., in `<string>`.) It seems likely that there's some master list of "libc++ headers" somewhere and that `<ext/__hash>` and `<ext/hash_map>` are accidentally missing from that list.
> > > > I thought the `<ext/*>` headers weren't in `module.modulemap`, because they were deprecated. It's also possible they are not in there by accident.
> > > I don't understand why this diff is necessary in this patch. What happens if you remove it? What fails?
> > Because we replace `-isystem` with `-I`. All warnings in headers are now part of the tests, so there must not be any warnings because of `-Werror`. So basically, the modules build would fail.
> But there's a `#pragma system_header`?
> I thought the `<ext/*>` headers weren't in `module.modulemap`, because they were deprecated. It's also possible they are not in there by accident.

I hypothesize that they're missing only by accident. I've posted D120758, and I'd be interested to see if you can eliminate these diffs by rebasing on it (or just adopting its diffs into this PR; I'll be happy to abandon it).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118616