-
Notifications
You must be signed in to change notification settings - Fork 13.6k
[libc++] Enable availability based on the compiler instead of __has_extension #84065
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…xtension __has_extension(...) doesn't work as intended when -pedantic-errors is used with Clang. With that flag, __has_extension(...) is equivalent to __has_feature(...), which means that checks like __has_extension(pragma_clang_attribute_external_declaration) will return 0. In turn, this has the effect of disabling availability markup in libc++, which is undesirable. rdar://124078119
@llvm/pr-subscribers-libcxx Author: Louis Dionne (ldionne) Changes__has_extension(...) doesn't work as intended when -pedantic-errors is used with Clang. With that flag, __has_extension(...) is equivalent to __has_feature(...), which means that checks like
will return 0. In turn, this has the effect of disabling availability markup in libc++, which is undesirable. rdar://124078119 Full diff: https://github.com/llvm/llvm-project/pull/84065.diff 2 Files Affected:
diff --git a/libcxx/include/__availability b/libcxx/include/__availability
index 78438c55a3b7ba..bb3ed0a8da521b 100644
--- a/libcxx/include/__availability
+++ b/libcxx/include/__availability
@@ -72,11 +72,10 @@
# endif
#endif
-// Availability markup is disabled when building the library, or when the compiler
+// Availability markup is disabled when building the library, or when a non-Clang
+// compiler is used because only Clang supports the necessary attributes.
// doesn't support the proper attributes.
-#if defined(_LIBCPP_BUILDING_LIBRARY) || defined(_LIBCXXABI_BUILDING_LIBRARY) || \
- !__has_feature(attribute_availability_with_strict) || !__has_feature(attribute_availability_in_templates) || \
- !__has_extension(pragma_clang_attribute_external_declaration)
+#if defined(_LIBCPP_BUILDING_LIBRARY) || defined(_LIBCXXABI_BUILDING_LIBRARY) || !defined(_LIBCPP_COMPILER_CLANG_BASED)
# if !defined(_LIBCPP_HAS_NO_VENDOR_AVAILABILITY_ANNOTATIONS)
# define _LIBCPP_HAS_NO_VENDOR_AVAILABILITY_ANNOTATIONS
# endif
diff --git a/libcxx/test/libcxx/vendor/apple/availability-with-pedantic-errors.compile.pass.cpp b/libcxx/test/libcxx/vendor/apple/availability-with-pedantic-errors.compile.pass.cpp
new file mode 100644
index 00000000000000..c55a0a4d6e5d1b
--- /dev/null
+++ b/libcxx/test/libcxx/vendor/apple/availability-with-pedantic-errors.compile.pass.cpp
@@ -0,0 +1,22 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// REQUIRES: stdlib=apple-libc++
+
+// Test that using -pedantic-errors doesn't turn off availability annotations.
+// This used to be the case because we used __has_extension(...) to enable the
+// availability annotations, and -pedantic-errors changes the behavior of
+// __has_extension(...) in an incompatible way.
+
+// ADDITIONAL_COMPILE_FLAGS: -pedantic-errors
+
+#include <__availability>
+
+#if defined(_LIBCPP_HAS_NO_VENDOR_AVAILABILITY_ANNOTATIONS)
+# error Availability annotations should be enabled on Apple platforms in the system configuration!
+#endif
|
I feel like we should fix this in Clang rather than in libc++. I think this behaviour is really surprising to people. CC @AaronBallman |
I could agree with that. Here's the analysis I did elsewhere, pasted: The problem seems to be that
And I am able to reproduce the cause for that here locally with:
I just found this in
And in
So basically, it looks like |
It's not that |
No, it kind of does disable extensions, which is why
should work. If |
I get the idea, but it doesn't work in practice. The (at least IMO) moral equivalents |
Sure, I think |
Given that it's just been added in GCC trunk (I'm quite surprised that got in) and the documentation says that it's for compatibility with clang, this isn't really surprising. |
I was surprised at how recently it was added to GCC, but what did surprise me was that the behavior was consistent between |
Do you mean compared to clang?
Absolutely. It seems we agree that something should change. Now the question is: what should change? Should we try to find all the possible ways to write |
Yup! (Sorry, that was horribly unclear.)
I can see arguments either way. The argument for making I think changing the behavior of On balance, I think |
From a standards library perspective getting answers to the first question is much more interesting, since most diagnostics won't be issued to users anyways.
Breaking code is definitely not great, especially changing documented behaviour. OTOH: What do users expect? In my experience people tend to not read the documentation of their compilers and standard libraries very closely (I'm very much guilty of that myself).
Extending the question to All in all, I think answers to both questions can be interesting. I'm just not sure how to answer the second without having a million special cases. Here are a few cases where I'm not sure what the correct answer should be:
They are quite related (other than the first) and it seems to me like we may want to produce different results based on how they're used. I'm also not sure how one would test things properly if |
Personally, I view I do understand the desire to check whether using an extension is going to produce an error, but I think that desire is misguided in most cases -- what people really want to know is whether the compiler they use supports the extension. In case you're writing a library and you want to allow people to compile with pedantic diagnostics, you should
|
Okay, I'm changing my opinion based on this discussion -- thank you for the extra feedback! I found these points to be particularly compelling:
and especially:
I still think all of the various feature testing macros should behave the same way, but now I'm leaning towards making them all behave more like |
Yeah, I think that's fair. We could (and should) also try it out on large code bases -- I can certainly help doing that over here if someone provides a Clang patch. I would like to still move forward with this libc++ change independently though, since it is correct and will fix this issue immediately, which is important for anyone using libc++ with |
Thank you for the offer! I can't promise I'll have time to do that patch myself in the near future, but I can promise to review it if someone wants to work on it (even if it's just an experiment without a public PR at first).
That sounds totally reasonable to me! |
I summarized the situation in #84372. |
I will cherry-pick this onto |
/cherry-pick 292a28d |
…xtension (llvm#84065) __has_extension(...) doesn't work as intended when -pedantic-errors is used with Clang. With that flag, __has_extension(...) is equivalent to __has_feature(...), which means that checks like __has_extension(pragma_clang_attribute_external_declaration) will return 0. In turn, this has the effect of disabling availability markup in libc++, which is undesirable. rdar://124078119 (cherry picked from commit 292a28d)
/pull-request #84374 |
…xtension (llvm#84065) __has_extension(...) doesn't work as intended when -pedantic-errors is used with Clang. With that flag, __has_extension(...) is equivalent to __has_feature(...), which means that checks like __has_extension(pragma_clang_attribute_external_declaration) will return 0. In turn, this has the effect of disabling availability markup in libc++, which is undesirable. rdar://124078119 (cherry picked from commit 292a28d)
__has_extension(...) doesn't work as intended when -pedantic-errors is used with Clang. With that flag, __has_extension(...) is equivalent to __has_feature(...), which means that checks like
will return 0. In turn, this has the effect of disabling availability markup in libc++, which is undesirable.
rdar://124078119