Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

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

Authored by nicolerabjohn on Mar 31 2023, 1:37 PM.



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.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
philnik added inline comments.Mar 31 2023, 1:45 PM

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

14 ↗(On Diff #510124)

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

This revision now requires changes to proceed.Mar 31 2023, 1:45 PM

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

nicolerabjohn updated this revision to Diff 511440.EditedApr 6 2023, 9:19 AM

Updating implementation by putting macros within LIBCPP_PUSH_MACROS and LIBCPP_POP_MACROS

nicolerabjohn edited the summary of this revision. (Show Details)Apr 6 2023, 9:58 AM

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


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.


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

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>

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

Updating formatting.

nicolerabjohn marked 3 inline comments as done.Apr 11 2023, 8:53 AM

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.


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


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.

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.

nicolerabjohn retitled this revision from Fixing conflicting macro definitions between curses.h and std::filesystem. to Fixing conflicting macro definitions between curses.h and the standard library..Apr 11 2023, 8:57 AM
nicolerabjohn edited the summary of this revision. (Show Details)
Mordante added inline comments.Apr 11 2023, 9:24 AM
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 marked 2 inline comments as done.Apr 11 2023, 10:07 AM
nicolerabjohn added inline comments.
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.

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

Updating diff with more context.

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

philnik requested changes to this revision.May 1 2023, 4:20 PM

Please make sure the CI is green.

145–150 ↗(On Diff #514651)

I think this should just be

#ifndef erase
#. define erase NASTY_MACRO

to make sure we always push and pop properly.

This revision now requires changes to proceed.May 1 2023, 4:20 PM

Updating testcase to use NASTY_MACROS

ldionne accepted this revision.May 10 2023, 10:35 AM
ldionne added a subscriber: ldionne.

LGTM w/ comment and green CI.

146 ↗(On Diff #518812)

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 accepted this revision.May 12 2023, 9:06 AM

LGTM with green CI.

This revision is now accepted and ready to land.May 12 2023, 9:06 AM

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.

Fixing failing test case.

Fix final test cases.

ldionne requested changes to this revision.Jul 4 2023, 12:34 PM

I spotted a couple of issues with the new patch.


Please move this right after

#  pragma GCC system_header

like for the other headers.


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


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.

249–250 ↗(On Diff #535896)

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

134 ↗(On Diff #535896)

This shouldn't be part of this patch.

This revision now requires changes to proceed.Jul 4 2023, 12:34 PM

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

nicolerabjohn marked 3 inline comments as done.Jul 5 2023, 12:27 PM
nicolerabjohn added inline comments.

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

ldionne accepted this revision.Jul 5 2023, 2:25 PM

Thanks, LGTM!

This revision is now accepted and ready to land.Jul 5 2023, 2:25 PM

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

nicolerabjohn marked an inline comment as done.

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

Syncing to arcanist

This revision was landed with ongoing or failed builds.Jul 6 2023, 10:21 AM
This revision was automatically updated to reflect the committed changes.