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

[Mark de Wever via Phabricator via libcxx-commits]

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



================
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()\")")
 
----------------
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.


================
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
----------------
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>`?


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



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

https://reviews.llvm.org/D147356