[libcxx-commits] [PATCH] D147356: Fixing conflicting macro definitions between curses.h and std::filesystem.

[Nicole Rabjohn via Phabricator via libcxx-commits]

nicolerabjohn marked 3 inline comments as done.
nicolerabjohn added a comment.

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



================
Comment at: libcxx/include/__config:1149
+    _Pragma("pop_macro(\"min\")") _Pragma("pop_macro(\"max\")") _Pragma("pop_macro(\"refresh()\")")                    \
+        _Pragma("pop_macro(\"move(int, int)\")") _Pragma("pop_macro(\"erase()\")")
 
----------------
Mordante wrote:
> 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.
I agree with the formatting here, updated. I had run clang-format on this, but I agree it was not very readable. 


================
Comment at: libcxx/include/__undef_macros:18-26
+#ifdef refresh
+#  if defined(__STDC__) || !defined(_NO_PROTO)
+inline int refresh_impl() { return refresh(); }
+#  else
+inline extern int refresh_impl() { return refresh(); }
+#  endif
+#  undef refresh
----------------
Mordante wrote:
> philnik wrote:
> > These should just be `#undef`ed 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>`.
> Why is this only an issue for filesystem and not for `<vector>`?
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. 


================
Comment at: libcxx/test/libcxx/vendor/ibm/curses_filesystem_macro_conflict.compile.pass.cpp:11
+
+#include "curses.h"
+#include <filesystem>
----------------
Mordante wrote:
> 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.
> 
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. 


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

https://reviews.llvm.org/D147356