Skip to content
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

[libc++] Use a different smart ptr type alias #102089

Merged
merged 1 commit into from
Aug 13, 2024

Conversation

androm3da
Copy link
Member

The _SP type is used by some C libraries and this alias could conflict with it.

@androm3da androm3da requested a review from a team as a code owner August 6, 2024 01:42
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Aug 6, 2024
@llvmbot
Copy link
Member

llvmbot commented Aug 6, 2024

@llvm/pr-subscribers-libcxx

Author: Brian Cain (androm3da)

Changes

The _SP type is used by some C libraries and this alias could conflict with it.


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

2 Files Affected:

  • (modified) libcxx/include/__memory/inout_ptr.h (+5-5)
  • (modified) libcxx/include/__memory/out_ptr.h (+4-4)
diff --git a/libcxx/include/__memory/inout_ptr.h b/libcxx/include/__memory/inout_ptr.h
index 72e1a21ad6867..e5f3ac5d027e8 100644
--- a/libcxx/include/__memory/inout_ptr.h
+++ b/libcxx/include/__memory/inout_ptr.h
@@ -63,17 +63,17 @@ class _LIBCPP_TEMPLATE_VIS inout_ptr_t {
       }
     }
 
-    using _SP = __pointer_of_or_t<_Smart, _Pointer>;
+    using _SmartPtr = __pointer_of_or_t<_Smart, _Pointer>;
     if constexpr (is_pointer_v<_Smart>) {
-      std::apply([&](auto&&... __args) { __s_ = _Smart(static_cast<_SP>(__p_), std::forward<_Args>(__args)...); },
+      std::apply([&](auto&&... __args) { __s_ = _Smart(static_cast<_SmartPtr>(__p_), std::forward<_Args>(__args)...); },
                  std::move(__a_));
     } else if constexpr (__resettable_smart_pointer_with_args<_Smart, _Pointer, _Args...>) {
-      std::apply([&](auto&&... __args) { __s_.reset(static_cast<_SP>(__p_), std::forward<_Args>(__args)...); },
+      std::apply([&](auto&&... __args) { __s_.reset(static_cast<_SmartPtr>(__p_), std::forward<_Args>(__args)...); },
                  std::move(__a_));
     } else {
-      static_assert(is_constructible_v<_Smart, _SP, _Args...>,
+      static_assert(is_constructible_v<_Smart, _SmartPtr, _Args...>,
                     "The smart pointer must be constructible from arguments of types _Smart, _Pointer, _Args...");
-      std::apply([&](auto&&... __args) { __s_ = _Smart(static_cast<_SP>(__p_), std::forward<_Args>(__args)...); },
+      std::apply([&](auto&&... __args) { __s_ = _Smart(static_cast<_SmartPtr>(__p_), std::forward<_Args>(__args)...); },
                  std::move(__a_));
     }
   }
diff --git a/libcxx/include/__memory/out_ptr.h b/libcxx/include/__memory/out_ptr.h
index 95aa2029c9231..fd99110790cc8 100644
--- a/libcxx/include/__memory/out_ptr.h
+++ b/libcxx/include/__memory/out_ptr.h
@@ -58,14 +58,14 @@ class _LIBCPP_TEMPLATE_VIS out_ptr_t {
       return;
     }
 
-    using _SP = __pointer_of_or_t<_Smart, _Pointer>;
+    using _SmartPtr = __pointer_of_or_t<_Smart, _Pointer>;
     if constexpr (__resettable_smart_pointer_with_args<_Smart, _Pointer, _Args...>) {
-      std::apply([&](auto&&... __args) { __s_.reset(static_cast<_SP>(__p_), std::forward<_Args>(__args)...); },
+      std::apply([&](auto&&... __args) { __s_.reset(static_cast<_SmartPtr>(__p_), std::forward<_Args>(__args)...); },
                  std::move(__a_));
     } else {
-      static_assert(is_constructible_v<_Smart, _SP, _Args...>,
+      static_assert(is_constructible_v<_Smart, _SmartPtr, _Args...>,
                     "The smart pointer must be constructible from arguments of types _Smart, _Pointer, _Args...");
-      std::apply([&](auto&&... __args) { __s_ = _Smart(static_cast<_SP>(__p_), std::forward<_Args>(__args)...); },
+      std::apply([&](auto&&... __args) { __s_ = _Smart(static_cast<_SmartPtr>(__p_), std::forward<_Args>(__args)...); },
                  std::move(__a_));
     }
   }

@androm3da
Copy link
Member Author

androm3da commented Aug 6, 2024

_SP is a preprocessor define in dinkumware libc's ctype.h. It might be nice to change this recently-introduced alias to avoid collisions when building libc++.

e.g. here is someone's copy of this C library: https://github.com/wuzhouhui/c_standard_lib/blob/e11548ad943686d303f0029d71ac2526d8b39cc5/_HEADERS/CTYPE.H#L12

cc @H-G-Hristov

Copy link
Member

@mordante mordante left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this!

Can you update the test libcxx/test/libcxx/system_reserved_names.gen.py with these ctype.h macros? That avoids us to use these names in the future.

@androm3da
Copy link
Member Author

Thanks for looking into this!

Can you update the test libcxx/test/libcxx/system_reserved_names.gen.py with these ctype.h macros? That avoids us to use these names in the future.

done

Copy link

github-actions bot commented Aug 6, 2024

✅ With the latest revision this PR passed the Python code formatter.

The `_SP` type is used by some C libraries and this alias could conflict
with it.
@Zingam
Copy link
Contributor

Zingam commented Aug 7, 2024

_SP is a preprocessor define in dinkumware libc's ctype.h. It might be nice to change this recently-introduced alias to avoid collisions when building libc++.

e.g. here is someone's copy of this C library: https://github.com/wuzhouhui/c_standard_lib/blob/e11548ad943686d303f0029d71ac2526d8b39cc5/_HEADERS/CTYPE.H#L12

cc @H-G-Hristov

Thank you! LGTM. IMO. This should be cherrypicked into LLVM 19 too.

@Zingam Zingam added this to the LLVM 19.X Release milestone Aug 7, 2024
@androm3da androm3da requested a review from mordante August 7, 2024 18:41
Copy link
Member

@mordante mordante left a comment

Choose a reason for hiding this comment

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

Thanks LGTM!

@androm3da androm3da merged commit 7951673 into llvm:main Aug 13, 2024
55 checks passed
@androm3da
Copy link
Member Author

/cherry-pick baa72c4

@llvmbot
Copy link
Member

llvmbot commented Aug 13, 2024

Failed to cherry-pick: baa72c4

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

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

@androm3da
Copy link
Member Author

Failed to cherry-pick: baa72c4

whoops wrong SHA. I will fix it.

@androm3da
Copy link
Member Author

/cherry-pick 7951673

llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Aug 13, 2024
The `_SP` type is used by some C libraries and this alias could conflict
with it.

(cherry picked from commit 7951673)
@llvmbot
Copy link
Member

llvmbot commented Aug 13, 2024

/pull-request #103003

androm3da added a commit to llvmbot/llvm-project that referenced this pull request Aug 14, 2024
The `_SP` type is used by some C libraries and this alias could conflict
with it.

(cherry picked from commit 7951673)
bwendling pushed a commit to bwendling/llvm-project that referenced this pull request Aug 15, 2024
The `_SP` type is used by some C libraries and this alias could conflict
with it.
tru pushed a commit to llvmbot/llvm-project that referenced this pull request Aug 15, 2024
The `_SP` type is used by some C libraries and this alias could conflict
with it.

(cherry picked from commit 7951673)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
Development

Successfully merging this pull request may close these issues.

5 participants