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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added a comment.

In D118616#3333754 <https://reviews.llvm.org/D118616#3333754>, @Quuxplusone wrote:

> LGTM mod comments, but I consider this to be @ldionne's jurisdiction, and it certainly doesn't seem super urgent, so I think we should just wait for him to take a look.
>
> Can you explain in the commit message how this relates to clang-tidy? Even looking at https://reviews.llvm.org/D118616?vs=404892&id=409993#toc I don't really understand your comment "Also fix clang-tidy in this patch."

Louis opened this PR before I landed the clang-tidy PR and this happens to be the fix for clang-tidy ignoring the headers. With the "Also fix clang-tidy in this patch" I meant that I'm fixing the warnings that clang-tidy produced after rebasing. I'm not sure what to add to the commit message. It's exactly the same problem, only that it's not clang producing the warnings, but clang-tidy.



================
Comment at: libcxx/include/__config_site.in:39
+#  pragma clang diagnostic push
+#  pragma clang diagnostic ignored "-Wmacro-redefined"
+#endif
----------------
mstorsjo wrote:
> For the cases where this was needed, the safest fix would be to make sure to include libcxx headers before standard c headers. Or alternatively we could maybe consider if `_LIBCPP_EXTRA_SITE_DEFINES` should add an `#undef` before each define.
I don't think the first option makes sense. That would mean that it makes a difference in which order the headers are included, which I'm not a fan of. The second one is an option, but I didn't have any luck with it yet.


================
Comment at: libcxx/include/__iterator/iterator_traits.h:201
     { __i -  __n } -> same_as<_Ip>;
-    { __i -  __i } -> same_as<decltype(__n)>;
+    { __i -  __i } -> same_as<decltype(__n)>; // NOLINT(misc-redundant-expression)
     {  __i[__n]  } -> convertible_to<iter_reference_t<_Ip>>;
----------------
Quuxplusone wrote:
> Please file a bug for this (I assume a clang-tidy bug, right?) and link it from here and/or in the commit message.
clang-tidy is kind-of right. `__i - __i` is a redundant expression, and I had to look at it a few times before I got why it's correct in this case. Where else would subtracting something from itself make sense?


================
Comment at: libcxx/include/ext/__hash:19-22
+_LIBCPP_DIAGNOSTIC_PUSH
+_LIBCPP_CLANG_DIAGNOSTIC_IGNORED("-Wprivate-header")
+#include <__string>
+_LIBCPP_DIAGNOSTIC_POP
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118616