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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added a comment.

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."



================
Comment at: libcxx/include/__config:94-96
+// These macros are defined in some tests
+_LIBCPP_DIAGNOSTIC_PUSH
+_LIBCPP_CLANG_DIAGNOSTIC_IGNORED("-Wmacro-redefined")
----------------
Then personally I'd prefer to pass `-Wno-macro-redefined` in those specific tests. We shouldn't do anything in `<__config>` that is beneficial only for certain tests in the test suite; we should do it here iff we think it'll benefit real users. Do we want real users to be able to do, like, `-D_LIBCPP_ABI_ALTERNATE_STRING_LAYOUT=1`?  If so, then lines 95-96 are good but the comment on line 94 should talk about //that//, not about tests in the test suite.


================
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>>;
----------------
Please file a bug for this (I assume a clang-tidy bug, right?) and link it from here and/or in the commit message.


================
Comment at: libcxx/include/__ranges/reverse_view.h:113-129
     template<class _Tp>
-    constexpr bool __is_reverse_view = false;
+    inline constexpr bool __is_reverse_view = false;
 
     template<class _Tp>
-    constexpr bool __is_reverse_view<reverse_view<_Tp>> = true;
+    inline constexpr bool __is_reverse_view<reverse_view<_Tp>> = true;
 
     template<class _Tp>
----------------
These changes LGTM but should be committed separately from whatever else is going on here.
You can just say "Reviewed as part of D118616". Or I'll do it myself if I get around to it tomorrow morning. :)


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


================
Comment at: libcxx/test/std/containers/sequences/array/size_and_alignment.pass.cpp:16
 
+// Ignore error about requesting a large alignment not being ABI compatible with older AIX systems.
+#if defined(_AIX)
----------------
I might think this also needs to be guarded by `&& defined(__clang__)`, but I suppose we never test any non-Clang compilers //on AIX//. Okay.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118616