[libcxx-commits] [PATCH] D77519: Fix __is_pointer builtin type trait to work with Objective-C pointer types.

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver marked 2 inline comments as done.
zoecarver added inline comments.


================
Comment at: clang/test/SemaObjCXX/type-traits-is-pointer.mm:1
+// RUN: %clang_cc1 -fsyntax-only -fobjc-arc -fobjc-runtime-has-weak -verify %s
+// expected-no-diagnostics
----------------
ldionne wrote:
> ldionne wrote:
> > Why do you run this through `-verify`? Can't you just drop that and the `// expected-no-diagnostics` bit too?
> Actually, never mind this. Someone pointed to me offline that using `clang-verify` has the benefit that no other diagnostics are going to be emitted, which is true. This is fine by me.
Great, thanks for the review. Most of these tests seem to use `-verify` so that's what I did. 


================
Comment at: libcxx/include/type_traits:901
+// In clang 10.0.0 and earlier __is_pointer didn't work with Objective-C types.
+#if __has_keyword(__is_pointer) && _LIBCPP_CLANG_VER > 1000
+
----------------
ldionne wrote:
> zoecarver wrote:
> > ldionne wrote:
> > > Doesn't `__has_keyword` return a number that can be compared against? `#if __has_keyword(__is_pointer) > some-number` would be better if feasible, because it would handle Clang, AppleClang and any other potential derivative.
> > I don't think `__has_keyword` returns a comparable number. Libc++ defines `__has_keyword ` using `__is_identifier` [1] and, it seems like all the feature checking macros from clang (including `__is_identifier`) have bool return types [2]. I can add an AppleClang check too if you want. 
> > 
> > [1]:
> > ```
> > #define __has_keyword(__x) !(__is_identifier(__x))
> > ```
> > 
> > [2]: https://clang.llvm.org/docs/LanguageExtensions.html#feature-checking-macros
> Ah, you're right. There's no version of AppleClang to put here right now, so I'd say status quo is OK. I can add one later.
Want me to add a TODO for you?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77519