[PATCH] D45470: Emit an error when mixing and

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai updated this revision to Diff 145817.
vsapsai added a comment.

Here is another approach that should emit an error only when mixing headers
causes compilation problems.

Have no ideas how to test the change. `-verify` doesn't work with fatal errors
and libcxx doesn't use FileCheck. Performed only manual testing.


https://reviews.llvm.org/D45470

Files:
  libcxx/include/atomic


Index: libcxx/include/atomic
===
--- libcxx/include/atomic
+++ libcxx/include/atomic
@@ -555,6 +555,9 @@
 #if !defined(_LIBCPP_HAS_C_ATOMIC_IMP) && !defined(_LIBCPP_HAS_GCC_ATOMIC_IMP)
 #error  is not implemented
 #endif
+#ifdef kill_dependency
+#error C++ standard library is incompatible with 
+#endif
 
 #if _LIBCPP_STD_VER > 14
 # define __cpp_lib_atomic_is_always_lock_free 201603L


Index: libcxx/include/atomic
===
--- libcxx/include/atomic
+++ libcxx/include/atomic
@@ -555,6 +555,9 @@
 #if !defined(_LIBCPP_HAS_C_ATOMIC_IMP) && !defined(_LIBCPP_HAS_GCC_ATOMIC_IMP)
 #error  is not implemented
 #endif
+#ifdef kill_dependency
+#error C++ standard library is incompatible with 
+#endif
 
 #if _LIBCPP_STD_VER > 14
 # define __cpp_lib_atomic_is_always_lock_free 201603L
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45470: Emit an error when mixing and

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai reopened this revision.
vsapsai added a comment.
This revision is now accepted and ready to land.

`__ALLOW_STDC_ATOMICS_IN_CXX__` approach didn't work in practice, I've reverted 
all changes.


Repository:
  rC Clang

https://reviews.llvm.org/D45470



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45470: Emit an error when mixing and

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

In https://reviews.llvm.org/D45470#1087026, @jfb wrote:

> This isn't bad, so I'd go with it, but separately I imagine that we could 
> implement the suggestion in http://wg21.link/p0943 and expose it even before 
> C++20? Not sure we do this much, but I'd argue that before that fix 
> stdatomic.h is just useless anyways, so it's a fine breakage. I imagine that 
> the stdatomic.h where it's implemented would be the one injected by clang, 
> not the libc++ one?


Change visible here is for the header injected by clang. libc++ change is 
r331379  but that error triggers only when 
you consciously decided to opt in C atomics but included C++ ``.

I am concerned that the change adds the error to the valid code that uses C 
atomics from C++ and doesn't mix them with C++ atomics. It is unfortunate but I 
think it is a right trade-off. Adding `__ALLOW_STDC_ATOMICS_IN_CXX__` to fix 
such code shouldn't take much time. While having an explicit error can save a 
lot of time trying to figure out a broken build. Also I expect that with time 
libc++ will use `` more and mixing currently safe parts of libc++ with 
`` is likely to cause problems at some point. So it is better to 
be prepared and have an explicit error for this case.


Repository:
  rC Clang

https://reviews.llvm.org/D45470



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45470: Emit an error when mixing and

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

This isn't bad, so I'd go with it, but separately I imagine that we could 
implement the suggestion in http://wg21.link/p0943 and expose it even before 
C++20? Not sure we do this much, but I'd argue that before that fix stdatomic.h 
is just useless anyways, so it's a fine breakage. I imagine that the 
stdatomic.h where it's implemented would be the one injected by clang, not the 
libc++ one?


Repository:
  rC Clang

https://reviews.llvm.org/D45470



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45470: Emit an error when mixing and

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

Thanks for the review, Marshall.

In the end I removed `XFAIL: with_system_cxx_lib` from libc++ test as the 
change is header-only at compile time. Library behaviour at runtime shouldn't 
be affected.


Repository:
  rC Clang

https://reviews.llvm.org/D45470



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45470: Emit an error when mixing and

[Volodymyr Sapsai via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC331378: Emit an error when mixing  and 
 (authored by vsapsai, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D45470?vs=143817&id=144903#toc

Repository:
  rC Clang

https://reviews.llvm.org/D45470

Files:
  lib/Headers/stdatomic.h
  test/Headers/stdatomic.cpp


Index: lib/Headers/stdatomic.h
===
--- lib/Headers/stdatomic.h
+++ lib/Headers/stdatomic.h
@@ -31,6 +31,10 @@
 # include_next 
 #else
 
+#if !defined(__ALLOW_STDC_ATOMICS_IN_CXX__) && defined(__cplusplus)
+#error " is incompatible with the C++ standard library; define 
__ALLOW_STDC_ATOMICS_IN_CXX__ to proceed."
+#endif
+
 #include 
 #include 
 
Index: test/Headers/stdatomic.cpp
===
--- test/Headers/stdatomic.cpp
+++ test/Headers/stdatomic.cpp
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 %s -verify
+// RUN: %clang_cc1 -D__ALLOW_STDC_ATOMICS_IN_CXX__ %s -verify
+
+#include 
+
+#ifndef __ALLOW_STDC_ATOMICS_IN_CXX__
+// expected-error@stdatomic.h:* {{ is incompatible with the C++ 
standard library}}
+#else
+// expected-no-diagnostics
+#endif


Index: lib/Headers/stdatomic.h
===
--- lib/Headers/stdatomic.h
+++ lib/Headers/stdatomic.h
@@ -31,6 +31,10 @@
 # include_next 
 #else
 
+#if !defined(__ALLOW_STDC_ATOMICS_IN_CXX__) && defined(__cplusplus)
+#error " is incompatible with the C++ standard library; define __ALLOW_STDC_ATOMICS_IN_CXX__ to proceed."
+#endif
+
 #include 
 #include 
 
Index: test/Headers/stdatomic.cpp
===
--- test/Headers/stdatomic.cpp
+++ test/Headers/stdatomic.cpp
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 %s -verify
+// RUN: %clang_cc1 -D__ALLOW_STDC_ATOMICS_IN_CXX__ %s -verify
+
+#include 
+
+#ifndef __ALLOW_STDC_ATOMICS_IN_CXX__
+// expected-error@stdatomic.h:* {{ is incompatible with the C++ standard library}}
+#else
+// expected-no-diagnostics
+#endif
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45470: Emit an error when mixing and

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai updated this revision to Diff 143817.
vsapsai added a comment.

- Tighten up a test with expected-error per review comment.


https://reviews.llvm.org/D45470

Files:
  clang/lib/Headers/stdatomic.h
  clang/test/Headers/stdatomic.cpp
  libcxx/include/atomic
  libcxx/test/libcxx/atomics/c_compatibility.fail.cpp


Index: libcxx/test/libcxx/atomics/c_compatibility.fail.cpp
===
--- /dev/null
+++ libcxx/test/libcxx/atomics/c_compatibility.fail.cpp
@@ -0,0 +1,36 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+//
+// UNSUPPORTED: libcpp-has-no-threads
+//
+// 
+
+// Test that including  fails to compile when we want to use C atomics
+// in C++ and have corresponding macro defined.
+
+// XFAIL: with_system_cxx_lib=macosx10.13
+// XFAIL: with_system_cxx_lib=macosx10.12
+// XFAIL: with_system_cxx_lib=macosx10.11
+// XFAIL: with_system_cxx_lib=macosx10.10
+// XFAIL: with_system_cxx_lib=macosx10.9
+// XFAIL: with_system_cxx_lib=macosx10.8
+// XFAIL: with_system_cxx_lib=macosx10.7
+
+// MODULES_DEFINES: __ALLOW_STDC_ATOMICS_IN_CXX__
+#ifndef __ALLOW_STDC_ATOMICS_IN_CXX__
+#define __ALLOW_STDC_ATOMICS_IN_CXX__
+#endif
+
+#include 
+// expected-error@atomic:* {{ is incompatible with the C++ 
standard library}}
+
+int main()
+{
+}
+
Index: libcxx/include/atomic
===
--- libcxx/include/atomic
+++ libcxx/include/atomic
@@ -555,6 +555,9 @@
 #if !defined(_LIBCPP_HAS_C_ATOMIC_IMP) && !defined(_LIBCPP_HAS_GCC_ATOMIC_IMP)
 #error  is not implemented
 #endif
+#ifdef __ALLOW_STDC_ATOMICS_IN_CXX__
+#error  is incompatible with the C++ standard library
+#endif
 
 #if _LIBCPP_STD_VER > 14
 # define __cpp_lib_atomic_is_always_lock_free 201603L
Index: clang/test/Headers/stdatomic.cpp
===
--- /dev/null
+++ clang/test/Headers/stdatomic.cpp
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 %s -verify
+// RUN: %clang_cc1 -D__ALLOW_STDC_ATOMICS_IN_CXX__ %s -verify
+
+#include 
+
+#ifndef __ALLOW_STDC_ATOMICS_IN_CXX__
+// expected-error@stdatomic.h:* {{ is incompatible with the C++ 
standard library}}
+#else
+// expected-no-diagnostics
+#endif
Index: clang/lib/Headers/stdatomic.h
===
--- clang/lib/Headers/stdatomic.h
+++ clang/lib/Headers/stdatomic.h
@@ -31,6 +31,10 @@
 # include_next 
 #else
 
+#if !defined(__ALLOW_STDC_ATOMICS_IN_CXX__) && defined(__cplusplus)
+#error " is incompatible with the C++ standard library; define 
__ALLOW_STDC_ATOMICS_IN_CXX__ to proceed."
+#endif
+
 #include 
 #include 
 


Index: libcxx/test/libcxx/atomics/c_compatibility.fail.cpp
===
--- /dev/null
+++ libcxx/test/libcxx/atomics/c_compatibility.fail.cpp
@@ -0,0 +1,36 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+//
+// UNSUPPORTED: libcpp-has-no-threads
+//
+// 
+
+// Test that including  fails to compile when we want to use C atomics
+// in C++ and have corresponding macro defined.
+
+// XFAIL: with_system_cxx_lib=macosx10.13
+// XFAIL: with_system_cxx_lib=macosx10.12
+// XFAIL: with_system_cxx_lib=macosx10.11
+// XFAIL: with_system_cxx_lib=macosx10.10
+// XFAIL: with_system_cxx_lib=macosx10.9
+// XFAIL: with_system_cxx_lib=macosx10.8
+// XFAIL: with_system_cxx_lib=macosx10.7
+
+// MODULES_DEFINES: __ALLOW_STDC_ATOMICS_IN_CXX__
+#ifndef __ALLOW_STDC_ATOMICS_IN_CXX__
+#define __ALLOW_STDC_ATOMICS_IN_CXX__
+#endif
+
+#include 
+// expected-error@atomic:* {{ is incompatible with the C++ standard library}}
+
+int main()
+{
+}
+
Index: libcxx/include/atomic
===
--- libcxx/include/atomic
+++ libcxx/include/atomic
@@ -555,6 +555,9 @@
 #if !defined(_LIBCPP_HAS_C_ATOMIC_IMP) && !defined(_LIBCPP_HAS_GCC_ATOMIC_IMP)
 #error  is not implemented
 #endif
+#ifdef __ALLOW_STDC_ATOMICS_IN_CXX__
+#error  is incompatible with the C++ standard library
+#endif
 
 #if _LIBCPP_STD_VER > 14
 # define __cpp_lib_atomic_is_always_lock_free 201603L
Index: clang/test/Headers/stdatomic.cpp
===
--- /dev/null
+++ clang/test/Headers/stdatomic.cpp
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 %s -verify
+// RUN: %clang_cc1 -D__ALLOW_STDC_AT

[PATCH] D45470: Emit an error when mixing and

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai planned changes to this revision.
vsapsai added inline comments.



Comment at: clang/test/Headers/stdatomic.cpp:4
+
+#include 
+

mclow.lists wrote:
> Is there a reason we want to test this twice - once in clang and once in 
> libc++?
> We can use `expected-error` in libc++ tests to check the error.
> 
The idea was to test `` in clang and `` in libc++. Does 
that answer your question? I'm not sure I understood it correctly.

Good suggestion to use `expected-error` in libc++ test, will add that.


https://reviews.llvm.org/D45470



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45470: Emit an error when mixing and

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added inline comments.



Comment at: clang/test/Headers/stdatomic.cpp:4
+
+#include 
+

Is there a reason we want to test this twice - once in clang and once in libc++?
We can use `expected-error` in libc++ tests to check the error.



https://reviews.llvm.org/D45470



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45470: Emit an error when mixing and

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.
Herald added a subscriber: jkorous.

Ping.


https://reviews.llvm.org/D45470



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45470: Emit an error when mixing and

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

In addition to the currently proposed approach

  #if ...
  #error ...
  #endif
  // header content

I also tried

  #if ...
  #error ...
  #else
  // header content
  #endif

It allows to have fewer error messages but can be more surprising. It can look 
like the only problem is a missing macro but after defining it you can discover 
that the situations is more complex. Also in some cases omitting a header 
content can lead to other errors which can be confusing too.


https://reviews.llvm.org/D45470



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45470: Emit an error when mixing and

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai created this revision.
vsapsai added reviewers: rsmith, EricWF, mclow.lists.
Herald added subscribers: christof, jkorous-apple.

Atomics in C and C++ are incompatible at the moment and mixing the
headers can result in confusing error messages.

Emit an error explicitly telling about the incompatibility. Introduce
the macro `__ALLOW_STDC_ATOMICS_IN_CXX__` that allows to choose in C++
between C atomics and C++ atomics.

rdar://problem/27435938


https://reviews.llvm.org/D45470

Files:
  clang/lib/Headers/stdatomic.h
  clang/test/Headers/stdatomic.cpp
  libcxx/include/atomic
  libcxx/test/libcxx/atomics/c_compatibility.fail.cpp


Index: libcxx/test/libcxx/atomics/c_compatibility.fail.cpp
===
--- /dev/null
+++ libcxx/test/libcxx/atomics/c_compatibility.fail.cpp
@@ -0,0 +1,35 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+//
+// UNSUPPORTED: libcpp-has-no-threads
+//
+// 
+
+// Test that including  fails to compile when we want to use C atomics
+// in C++ and have corresponding macro defined.
+
+// XFAIL: with_system_cxx_lib=macosx10.13
+// XFAIL: with_system_cxx_lib=macosx10.12
+// XFAIL: with_system_cxx_lib=macosx10.11
+// XFAIL: with_system_cxx_lib=macosx10.10
+// XFAIL: with_system_cxx_lib=macosx10.9
+// XFAIL: with_system_cxx_lib=macosx10.8
+// XFAIL: with_system_cxx_lib=macosx10.7
+
+// MODULES_DEFINES: __ALLOW_STDC_ATOMICS_IN_CXX__
+#ifndef __ALLOW_STDC_ATOMICS_IN_CXX__
+#define __ALLOW_STDC_ATOMICS_IN_CXX__
+#endif
+
+#include 
+
+int main()
+{
+}
+
Index: libcxx/include/atomic
===
--- libcxx/include/atomic
+++ libcxx/include/atomic
@@ -555,6 +555,9 @@
 #if !defined(_LIBCPP_HAS_C_ATOMIC_IMP) && !defined(_LIBCPP_HAS_GCC_ATOMIC_IMP)
 #error  is not implemented
 #endif
+#ifdef __ALLOW_STDC_ATOMICS_IN_CXX__
+#error  is incompatible with the C++ standard library
+#endif
 
 #if _LIBCPP_STD_VER > 14
 # define __cpp_lib_atomic_is_always_lock_free 201603L
Index: clang/test/Headers/stdatomic.cpp
===
--- /dev/null
+++ clang/test/Headers/stdatomic.cpp
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 %s -verify
+// RUN: %clang_cc1 -D__ALLOW_STDC_ATOMICS_IN_CXX__ %s -verify
+
+#include 
+
+#ifndef __ALLOW_STDC_ATOMICS_IN_CXX__
+// expected-error@stdatomic.h:* {{ is incompatible with the C++ 
standard library}}
+#else
+// expected-no-diagnostics
+#endif
Index: clang/lib/Headers/stdatomic.h
===
--- clang/lib/Headers/stdatomic.h
+++ clang/lib/Headers/stdatomic.h
@@ -31,6 +31,10 @@
 # include_next 
 #else
 
+#if !defined(__ALLOW_STDC_ATOMICS_IN_CXX__) && defined(__cplusplus)
+#error " is incompatible with the C++ standard library; define 
__ALLOW_STDC_ATOMICS_IN_CXX__ to proceed."
+#endif
+
 #include 
 #include 
 


Index: libcxx/test/libcxx/atomics/c_compatibility.fail.cpp
===
--- /dev/null
+++ libcxx/test/libcxx/atomics/c_compatibility.fail.cpp
@@ -0,0 +1,35 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+//
+// UNSUPPORTED: libcpp-has-no-threads
+//
+// 
+
+// Test that including  fails to compile when we want to use C atomics
+// in C++ and have corresponding macro defined.
+
+// XFAIL: with_system_cxx_lib=macosx10.13
+// XFAIL: with_system_cxx_lib=macosx10.12
+// XFAIL: with_system_cxx_lib=macosx10.11
+// XFAIL: with_system_cxx_lib=macosx10.10
+// XFAIL: with_system_cxx_lib=macosx10.9
+// XFAIL: with_system_cxx_lib=macosx10.8
+// XFAIL: with_system_cxx_lib=macosx10.7
+
+// MODULES_DEFINES: __ALLOW_STDC_ATOMICS_IN_CXX__
+#ifndef __ALLOW_STDC_ATOMICS_IN_CXX__
+#define __ALLOW_STDC_ATOMICS_IN_CXX__
+#endif
+
+#include 
+
+int main()
+{
+}
+
Index: libcxx/include/atomic
===
--- libcxx/include/atomic
+++ libcxx/include/atomic
@@ -555,6 +555,9 @@
 #if !defined(_LIBCPP_HAS_C_ATOMIC_IMP) && !defined(_LIBCPP_HAS_GCC_ATOMIC_IMP)
 #error  is not implemented
 #endif
+#ifdef __ALLOW_STDC_ATOMICS_IN_CXX__
+#error  is incompatible with the C++ standard library
+#endif
 
 #if _LIBCPP_STD_VER > 14
 # define __cpp_lib_atomic_is_always_lock_free 201603L
Index: clang/test/Headers/stdatomic.cpp
=