Fixing conflicting macro definitions between curses.h and the standard library.

[nicolerabjohn]

Summary

POSIX allows certain macros to exist with generic names (i.e. refresh(), move(), and erase()) to exist in curses.h which conflict with functions found in std::filesystem, among others. This patch undefs the macros in question and adds them to LIBCPP_PUSH_MACROS and LIBCPP_POP_MACROS.

[philnik]

libcxx/include/__undef_macros

18–26

These should just be #undefed and added to _LIBCPP_PUSH_MACROS and _LIBCPP_POP_MACROS. Arguably they should be replaced in the libc, since these names are used everywhere, not just in <filesystem>.

libcxx/test/libcxx/vendor/ibm/curses_macro_conflict.pass.cpp

14 ↗

(On Diff #510124)

This should just be a .compile.pass.cpp.

[nicolerabjohn]

Addressing reviewer comment to rename testcase and generalizing __undef_macros to work for all platforms.

[nicolerabjohn]

Updating implementation by putting macros within LIBCPP_PUSH_MACROS and LIBCPP_POP_MACROS

[Mordante]

Before approving I really like to know why this only affects <filesystem>. I'm slightly wary to add work-arounds for unrelated external libaries (even when curses is quite Standard).

libcxx/include/__config

1149

I really would like one entry per line, that makes the code a bit longer, but makes it easier to verify. Especially since the entries are not nicely aligned.

libcxx/include/__undef_macros

18–26

Why is this only an issue for filesystem and not for <vector>?

libcxx/test/libcxx/vendor/ibm/curses_filesystem_macro_conflict.compile.pass.cpp

11 ↗

(On Diff #511440)

Why are these IBM specific tests and not using a test feature has-curses?

Or why not something like

#if __has_include(<curses.h>)
#  include <curses.h>
#endif

Then the test could be included in libcxx/test/libcxx/nasty_macros.compile.pass.cpp instead of IBM only.

[nicolerabjohn]

Updating formatting.

[nicolerabjohn]

Before approving I really like to know why this only affects <filesystem>. I'm slightly wary to add work-arounds for unrelated external libaries (even when curses is quite Standard).

Curses in this cases isn't really just any "external" library per-say. It's part of the OS, and standardized by POSIX, so it possible for any compliant UNIX to have this problem depending on the implementation details of the header.

As of now, the conflicts between POSIX-compliant curses.h and these affected headers means that compilers using libc++ (clang, specifically, in this case) can't build CMake on AIX. This feels significant enough to add a workaround for.

libcxx/include/__config

1149

I agree with the formatting here, updated. I had run clang-format on this, but I agree it was not very readable.

libcxx/include/__undef_macros

18–26

It would in theory be an issue for <vector> - the only reason the review wasn't named as such was because the first issue that was noticed came out of <filesystem>. <vector> also already had the LIBCPP_PUSH_MACROS code implemented, so once I added the new macros to the pragma, conflicts within <vector> were fixed. I will update the title/description of the review to be more descriptive.

libcxx/test/libcxx/vendor/ibm/curses_filesystem_macro_conflict.compile.pass.cpp

11 ↗

(On Diff #511440)

So I agree that these don't need to be IBM-specific - libcxx/test/libcxx/nasty_macros.compile.pass.cpp might not be the best choice here either because it's currently XFAILED on AIX for unrelated reasons. AIX is affected by this patch, so I would like these test cases to run on AIX as well. I'll look around and see if I can fix the XFAIL, or find a more appropriate spot for these tests.

[Mordante]

libcxx/test/libcxx/vendor/ibm/curses_filesystem_macro_conflict.compile.pass.cpp

11 ↗

(On Diff #511440)

I think then we really should make sure the AIX tests are fixed, in a separate patch. The nasty macros are really intended for these tests. That also is used to avoid regressions. If a header doesn't push the macros and in C++26 is gets an erase function it will be caught in our CI.

[nicolerabjohn]

libcxx/test/libcxx/vendor/ibm/curses_filesystem_macro_conflict.compile.pass.cpp

11 ↗

(On Diff #511440)

Fair enough - I'll take a look at why it was XFAILed to see if it can be fixed, or if the problematic headers can be split out.

[nicolerabjohn]

There was a fix for nasty_macros.compile.pass.cpp in a recent AIX update. I've put up https://reviews.llvm.org/D148040 to revert the XFAILs. Once that lands, I'll update this patch to put the test cases there.

[nicolerabjohn]

Updating diff with more context.

[nicolerabjohn]

Updating test cases to use nasty_macros.compile.pass.cpp instead of IBM-specific tests.

[nicolerabjohn]

Gentle ping.

[philnik]

Please make sure the CI is green.

libcxx/test/libcxx/nasty_macros.compile.pass.cpp

145–150

I think this should just be

#ifndef erase
#. define erase NASTY_MACRO
#endif

to make sure we always push and pop properly.

[nicolerabjohn]

Updating testcase to use NASTY_MACROS

[ldionne]

LGTM w/ comment and green CI.

libcxx/test/libcxx/nasty_macros.compile.pass.cpp

146

You can just define all of those unconditionally since there's no header included before. Essentially we know the #ifndef is always true right now.

[philnik]

LGTM with green CI.

[nicolerabjohn]

Rebase

[nicolerabjohn]

Including __undef_macro in more header files - anywhere that move, refresh, or erase are used. This should fix the gcc compat problem.

Lots of files are changed here, so if there are any objections to this direction, I'm happy to hear them - otherwise, I will land in a few days if the CI goes green.

[nicolerabjohn]

Format fix.

[nicolerabjohn]

Fixing failing test case.

[nicolerabjohn]

Fix final test cases.

[ldionne]

I spotted a couple of issues with the new patch.

libcxx/include/optional

258–260 ↗

(On Diff #535896)

Please move this right after

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

like for the other headers.

1673 ↗

(On Diff #535896)

And this needs to go after #endif // _LIBCPP_STD_VER >= 17 then.

libcxx/include/tuple

1860–1862 ↗

(On Diff #535896)

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.

libcxx/include/utility

249–250 ↗

(On Diff #535896)

We shouldn't add it here, we only add it when required.

libcxx/utils/ci/run-buildbot

134 ↗

(On Diff #535896)

This shouldn't be part of this patch.

[nicolerabjohn]

Reordering certain code blocks, general clean-up of the patch.

[nicolerabjohn]

libcxx/include/tuple

1860–1862 ↗

(On Diff #535896)

I found a few, updated them all. They should all follow the same formatting now.

[ldionne]

Thanks, LGTM!

[ldionne]

If you need someone to commit this on your behalf, please let us know the Author Name <email@domain> to use for attribution.

[nicolerabjohn]

Fixing format for CI. Once this goes green, I'll land it.

[nicolerabjohn]

Syncing to arcanist

libcxx/include/__config

 #    define _LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_MEMBERS
 #    define _LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_VOID_SPECIALIZATION
 #    define _LIBCPP_ENABLE_CXX20_REMOVED_BINDER_TYPEDEFS
 #    define _LIBCPP_ENABLE_CXX20_REMOVED_NEGATORS
 #    define _LIBCPP_ENABLE_CXX20_REMOVED_RAW_STORAGE_ITERATOR
 #    define _LIBCPP_ENABLE_CXX20_REMOVED_TYPE_TRAITS
 #  endif // _LIBCPP_ENABLE_CXX20_REMOVED_FEATURES
 
-#  define _LIBCPP_PUSH_MACROS _Pragma("push_macro(\"min\")") _Pragma("push_macro(\"max\")")
-#  define _LIBCPP_POP_MACROS _Pragma("pop_macro(\"min\")") _Pragma("pop_macro(\"max\")")
+#  define _LIBCPP_PUSH_MACROS _Pragma("push_macro(\"min\")") _Pragma("push_macro(\"max\")") _Pragma("push_macro(\"refresh()\")") _Pragma("push_macro(\"move(int, int)\")") _Pragma("push_macro(\"erase()\")")
+#  define _LIBCPP_POP_MACROS _Pragma("pop_macro(\"min\")") _Pragma("pop_macro(\"max\")") _Pragma("pop_macro(\"refresh()\")") _Pragma("pop_macro(\"move(int, int)\")") _Pragma("pop_macro(\"erase()\")")
 
 #  ifndef _LIBCPP_NO_AUTO_LINK
 #    if defined(_LIBCPP_ABI_MICROSOFT) && !defined(_LIBCPP_BUILDING_LIBRARY)
 #      if !defined(_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS)
 #        pragma comment(lib, "c++.lib")
 #      else
 #        pragma comment(lib, "libc++.lib")
 #      endif
 #    endif // defined(_LIBCPP_ABI_MICROSOFT) && !defined(_LIBCPP_BUILDING_LIBRARY)
 #  endif   // _LIBCPP_NO_AUTO_LINK
 
 // Configures the fopen close-on-exec mode character, if any. This string will

libcxx/include/__undef_macros

 // -*- C++ -*-
 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifdef min
 #  undef min
 #endif
 
 #ifdef max
 #  undef max
 #endif
+
+#ifdef refresh
+#  undef refresh
+#endif
+
+#ifdef move
+#  undef move
+#endif
+
+#ifdef erase
+#  undef erase
+#endif

libcxx/include/__utility/move.h

 #include <__type_traits/is_copy_constructible.h>
 #include <__type_traits/is_nothrow_move_constructible.h>
 #include <__type_traits/remove_reference.h>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #  pragma GCC system_header
 #endif
 
+_LIBCPP_PUSH_MACROS
+#include <__undef_macros>
+
 _LIBCPP_BEGIN_NAMESPACE_STD
 
 template <class _Tp>
 _LIBCPP_NODISCARD_EXT inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR __libcpp_remove_reference_t<_Tp>&&
 move(_LIBCPP_LIFETIMEBOUND _Tp&& __t) _NOEXCEPT {
   typedef _LIBCPP_NODEBUG __libcpp_remove_reference_t<_Tp> _Up;
   return static_cast<_Up&&>(__t);
 }
 
 template <class _Tp>
 using __move_if_noexcept_result_t =
     __conditional_t<!is_nothrow_move_constructible<_Tp>::value && is_copy_constructible<_Tp>::value, const _Tp&, _Tp&&>;
 
 template <class _Tp>
 _LIBCPP_NODISCARD_EXT inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 __move_if_noexcept_result_t<_Tp>
 move_if_noexcept(_LIBCPP_LIFETIMEBOUND _Tp& __x) _NOEXCEPT {
   return std::move(__x);
 }
 
 _LIBCPP_END_NAMESPACE_STD
 
+_LIBCPP_POP_MACROS
+
 #endif // _LIBCPP___UTILITY_MOVE_H

libcxx/test/libcxx/nasty_macros.compile.pass.cpp

 #define X NASTY_MACRO
 #define Xp NASTY_MACRO
 #define Xs NASTY_MACRO
 
 // The classic Windows min/max macros
 #define min NASTY_MACRO
 #define max NASTY_MACRO
 
+// Test to make sure curses has no conflicting macros with the standard library
+#ifdef __has_include
+#  if __has_include(<curses.h>)
+#    include <curses.h>
+#  endif
+#endif
+
 /*
 BEGIN-SCRIPT
 
 for header in public_headers:
   print("{}#{}include <{}>{}".format(
     '#if ' + header_restrictions[header] + '\n' if header in header_restrictions else '',
     3 * ' ' if header in header_restrictions else '',
     header,