Skip to content

Conversation

@philnik777
Copy link
Contributor

This aligns the builtins with how implementations work which don't use the buitins.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jan 21, 2025
@llvmbot
Copy link
Member

llvmbot commented Jan 21, 2025

@llvm/pr-subscribers-clang

Author: Nikolas Klauser (philnik777)

Changes

This aligns the builtins with how implementations work which don't use the buitins.


Full diff: https://github.com/llvm/llvm-project/pull/123678.diff

2 Files Affected:

  • (modified) clang/lib/Sema/SemaType.cpp (+3-3)
  • (added) clang/test/SemaObjCXX/type-traits.mm (+14)
diff --git a/clang/lib/Sema/SemaType.cpp b/clang/lib/Sema/SemaType.cpp
index 2ccf5a8e1d6f31..10be75c53af08f 100644
--- a/clang/lib/Sema/SemaType.cpp
+++ b/clang/lib/Sema/SemaType.cpp
@@ -1826,7 +1826,8 @@ QualType Sema::BuildPointerType(QualType T,
   if (checkQualifiedFunction(*this, T, Loc, QFK_Pointer))
     return QualType();
 
-  assert(!T->isObjCObjectType() && "Should build ObjCObjectPointerType");
+  if (T->isObjCObjectType())
+    return Context.getObjCObjectPointerType(T);
 
   // In ARC, it is forbidden to build pointers to unqualified pointers.
   if (getLangOpts().ObjCAutoRefCount)
@@ -9806,8 +9807,7 @@ QualType Sema::BuiltinAddPointer(QualType BaseType, SourceLocation Loc) {
 }
 
 QualType Sema::BuiltinRemovePointer(QualType BaseType, SourceLocation Loc) {
-  // We don't want block pointers or ObjectiveC's id type.
-  if (!BaseType->isAnyPointerType() || BaseType->isObjCIdType())
+  if (!BaseType->isAnyPointerType())
     return BaseType;
 
   return BaseType->getPointeeType();
diff --git a/clang/test/SemaObjCXX/type-traits.mm b/clang/test/SemaObjCXX/type-traits.mm
new file mode 100644
index 00000000000000..20cb980e8880bf
--- /dev/null
+++ b/clang/test/SemaObjCXX/type-traits.mm
@@ -0,0 +1,14 @@
+// RUN: %clang_cc1 -triple x86_64-apple-darwin10 -fsyntax-only -verify -std=c++17  %s
+
+// expected-no-diagnostics
+
+@interface I;
+@end
+
+static_assert(__is_same(__add_pointer(id), id*));
+static_assert(__is_same(__add_pointer(I), I*));
+
+static_assert(!__is_same(__remove_pointer(id), id));
+static_assert(__is_same(__remove_pointer(id*), id));
+static_assert(__is_same(__remove_pointer(__add_pointer(id)), id));
+static_assert(__is_same(__add_pointer(__remove_pointer(id)), id));

Copy link
Contributor

@cor3ntin cor3ntin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a changelog entry?

Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oof, I'd like to hear from @rjmccall -- Objective-C pointer types are not C pointer types.

@rjmccall
Copy link
Contributor

It's probably right that these should match the behavior of adding or removing a * (the latter of which can happen via template argument deduction), which does mean they need to handle ObjC pointer types correctly.

@philnik777
Copy link
Contributor Author

@AaronBallman is there anything blocking this? I'd really like to have this in the release branch as early as possible.

Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@philnik777 philnik777 merged commit 74d7f43 into llvm:main Jan 30, 2025
5 of 7 checks passed
@philnik777 philnik777 deleted the fix_add_pointer branch January 30, 2025 19:34
@philnik777
Copy link
Contributor Author

/cherry-pick 74d7f43

@llvmbot
Copy link
Member

llvmbot commented Jan 30, 2025

/cherry-pick 74d7f43

Error: Command failed due to missing milestone.

@philnik777 philnik777 added this to the LLVM 20.X Release milestone Jan 30, 2025
@philnik777
Copy link
Contributor Author

/cherry-pick 74d7f43

@llvmbot
Copy link
Member

llvmbot commented Jan 30, 2025

Failed to cherry-pick: 74d7f43

https://github.com/llvm/llvm-project/actions/runs/13060196391

Please manually backport the fix and push it to your github fork. Once this is done, please create a pull request

philnik777 added a commit to philnik777/llvm-project that referenced this pull request Jan 31, 2025
This aligns the builtins with how implementations work which don't use
the buitins.
swift-ci pushed a commit to swiftlang/llvm-project that referenced this pull request Feb 11, 2025
This aligns the builtins with how implementations work which don't use
the buitins.
@tstellar
Copy link
Collaborator

@philnik777 Were you able to manually create the backport pull request?

@philnik777
Copy link
Contributor Author

@tstellar Yes, it's #125185.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category release:cherry-pick-failed

Projects

Development

Successfully merging this pull request may close these issues.

6 participants