[libcxx-commits] [PATCH] D147356: Fixing conflicting macro definitions between curses.h and the standard library.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

I spotted a couple of issues with the new patch.



================
Comment at: libcxx/include/optional:258-260
+_LIBCPP_PUSH_MACROS
+#include <__undef_macros>
+
----------------
Please move this right after 

```
#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
#  pragma GCC system_header
#endif
```

like for the other headers.


================
Comment at: libcxx/include/optional:1673
 
+_LIBCPP_POP_MACROS
+
----------------
And this needs to go after `#endif // _LIBCPP_STD_VER >= 17` then.


================
Comment at: libcxx/include/tuple:1860-1862
+_LIBCPP_POP_MACROS
+
 _LIBCPP_END_NAMESPACE_STD
----------------
This needs to go after `_LIBCPP_END_NAMESPACE_STD`, please audit the files you changed to make sure you have the right order. I don't think I saw other problematic ones but just to be sure.


================
Comment at: libcxx/include/utility:249-250
 
+#include <__undef_macros>
+
 #include <__assert> // all public C++ headers provide the assertion handler
----------------
We shouldn't add it here, we only add it when required.


================
Comment at: libcxx/utils/ci/run-buildbot:134
           -DLIBCXX_ENABLE_CLANG_TIDY=${ENABLE_CLANG_TIDY} \
-          -DLLVM_LIT_ARGS="-sv --show-unsupported --xunit-xml-output test-results.xml --timeout=1500 --time-tests" \
+          -DLLVM_LIT_ARGS="-sv --show-unsupported --xunit-xml-output test-results.xml" \
           "${@}"
----------------
This shouldn't be part of this patch.


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

https://reviews.llvm.org/D147356