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.
Details
- Reviewers
daltenty philnik ldionne - Group Reviewers
Restricted Project - Commits
- rG92e4d6791f71: Fixing conflicting macro definitions between curses.h and the standard library.
Diff Detail
Event Timeline
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. |
Addressing reviewer comment to rename testcase and generalizing __undef_macros to work for all platforms.
Updating implementation by putting macros within LIBCPP_PUSH_MACROS and LIBCPP_POP_MACROS
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. |
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. |
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. |
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. |
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.
Updating test cases to use nasty_macros.compile.pass.cpp instead of IBM-specific tests.
Please make sure the CI is green.
libcxx/test/libcxx/nasty_macros.compile.pass.cpp | ||
---|---|---|
145–150 ↗ | (On Diff #514651) | I think this should just be #ifndef erase #. define erase NASTY_MACRO #endif to make sure we always push and pop properly. |
LGTM w/ comment and green CI.
libcxx/test/libcxx/nasty_macros.compile.pass.cpp | ||
---|---|---|
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. |
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.
I spotted a couple of issues with the new patch.
libcxx/include/optional | ||
---|---|---|
258–260 | Please move this right after #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER) # pragma GCC system_header #endif like for the other headers. | |
1673 | And this needs to go after #endif // _LIBCPP_STD_VER >= 17 then. | |
libcxx/include/tuple | ||
1860–1862 | 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 | 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. |
libcxx/include/tuple | ||
---|---|---|
1860–1862 | I found a few, updated them all. They should all follow the same formatting now. |
If you need someone to commit this on your behalf, please let us know the Author Name <email@domain> to use for attribution.
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.