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

[libcxx] responds to Clang Tidy feedback #97556

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cjdb
Copy link
Contributor

@cjdb cjdb commented Jul 3, 2024

Clang Tidy flagged the following as problematic when running tests locally.

Clang Tidy flagged the following as problematic when running tests
locally.
@cjdb cjdb added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Jul 3, 2024
@cjdb cjdb requested a review from a team as a code owner July 3, 2024 11:17
@llvmbot
Copy link
Collaborator

llvmbot commented Jul 3, 2024

@llvm/pr-subscribers-libcxx

Author: Christopher Di Bella (cjdb)

Changes

Clang Tidy flagged the following as problematic when running tests locally.


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

3 Files Affected:

  • (modified) libcxx/include/__functional/function.h (+3-3)
  • (modified) libcxx/include/__thread/id.h (+8-8)
  • (modified) libcxx/include/__thread/support/pthread.h (+1-1)
diff --git a/libcxx/include/__functional/function.h b/libcxx/include/__functional/function.h
index c7b98035e34bf..cb6ad7352e716 100644
--- a/libcxx/include/__functional/function.h
+++ b/libcxx/include/__functional/function.h
@@ -754,7 +754,7 @@ class __func<_Rp1 (^)(_ArgTypes1...), _Alloc, _Rp(_ArgTypes...)> : public __base
 #    ifdef _LIBCPP_HAS_OBJC_ARC
       : __f_(__f)
 #    else
-      : __f_(reinterpret_cast<__block_type>(__f ? _Block_copy(__f) : nullptr))
+      : __f_(reinterpret_cast<__block_type>(__f ? std::__function::_Block_copy(__f) : nullptr))
 #    endif
   {
   }
@@ -765,7 +765,7 @@ class __func<_Rp1 (^)(_ArgTypes1...), _Alloc, _Rp(_ArgTypes...)> : public __base
 #    ifdef _LIBCPP_HAS_OBJC_ARC
       : __f_(__f)
 #    else
-      : __f_(reinterpret_cast<__block_type>(__f ? _Block_copy(__f) : nullptr))
+      : __f_(reinterpret_cast<__block_type>(__f ? std::__function::_Block_copy(__f) : nullptr))
 #    endif
   {
   }
@@ -786,7 +786,7 @@ class __func<_Rp1 (^)(_ArgTypes1...), _Alloc, _Rp(_ArgTypes...)> : public __base
   _LIBCPP_HIDE_FROM_ABI_VIRTUAL virtual void destroy() _NOEXCEPT {
 #    ifndef _LIBCPP_HAS_OBJC_ARC
     if (__f_)
-      _Block_release(__f_);
+      std::__function::_Block_release(__f_);
 #    endif
     __f_ = 0;
   }
diff --git a/libcxx/include/__thread/id.h b/libcxx/include/__thread/id.h
index 6db0ccbfe569b..6b014720ce05e 100644
--- a/libcxx/include/__thread/id.h
+++ b/libcxx/include/__thread/id.h
@@ -42,17 +42,17 @@ class _LIBCPP_TEMPLATE_VIS __thread_id {
 
   static _LIBCPP_HIDE_FROM_ABI bool
   __lt_impl(__thread_id __x, __thread_id __y) _NOEXCEPT { // id==0 is always less than any other thread_id
-    if (__x.__id_ == 0)
-      return __y.__id_ != 0;
-    if (__y.__id_ == 0)
+    if (__x.__id_ == nullptr)
+      return __y.__id_ != nullptr;
+    if (__y.__id_ == nullptr)
       return false;
     return __libcpp_thread_id_less(__x.__id_, __y.__id_);
   }
 
 public:
-  _LIBCPP_HIDE_FROM_ABI __thread_id() _NOEXCEPT : __id_(0) {}
+  _LIBCPP_HIDE_FROM_ABI __thread_id() _NOEXCEPT : __id_(nullptr) {}
 
-  _LIBCPP_HIDE_FROM_ABI void __reset() { __id_ = 0; }
+  _LIBCPP_HIDE_FROM_ABI void __reset() { __id_ = nullptr; }
 
   friend _LIBCPP_HIDE_FROM_ABI bool operator==(__thread_id __x, __thread_id __y) _NOEXCEPT;
 #  if _LIBCPP_STD_VER <= 17
@@ -77,9 +77,9 @@ class _LIBCPP_TEMPLATE_VIS __thread_id {
 
 inline _LIBCPP_HIDE_FROM_ABI bool operator==(__thread_id __x, __thread_id __y) _NOEXCEPT {
   // Don't pass id==0 to underlying routines
-  if (__x.__id_ == 0)
-    return __y.__id_ == 0;
-  if (__y.__id_ == 0)
+  if (__x.__id_ == nullptr)
+    return __y.__id_ == nullptr;
+  if (__y.__id_ == nullptr)
     return false;
   return __libcpp_thread_id_equal(__x.__id_, __y.__id_);
 }
diff --git a/libcxx/include/__thread/support/pthread.h b/libcxx/include/__thread/support/pthread.h
index 531f3e71de839..94ba471027ed0 100644
--- a/libcxx/include/__thread/support/pthread.h
+++ b/libcxx/include/__thread/support/pthread.h
@@ -175,7 +175,7 @@ inline _LIBCPP_HIDE_FROM_ABI __libcpp_thread_id __libcpp_thread_get_id(const __l
 }
 
 inline _LIBCPP_HIDE_FROM_ABI bool __libcpp_thread_isnull(const __libcpp_thread_t* __t) {
-  return __libcpp_thread_get_id(__t) == 0;
+  return __libcpp_thread_get_id(__t) == nullptr;
 }
 
 inline _LIBCPP_HIDE_FROM_ABI int __libcpp_thread_create(__libcpp_thread_t* __t, void* (*__func)(void*), void* __arg) {

@cjdb
Copy link
Contributor Author

cjdb commented Jul 3, 2024

This only seems to be an issue on my Mac. I get build errors on Linux?

@@ -754,7 +754,7 @@ class __func<_Rp1 (^)(_ArgTypes1...), _Alloc, _Rp(_ArgTypes...)> : public __base
# ifdef _LIBCPP_HAS_OBJC_ARC
: __f_(__f)
# else
: __f_(reinterpret_cast<__block_type>(__f ? _Block_copy(__f) : nullptr))
: __f_(reinterpret_cast<__block_type>(__f ? std::__function::_Block_copy(__f) : nullptr))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
: __f_(reinterpret_cast<__block_type>(__f ? std::__function::_Block_copy(__f) : nullptr))
: __f_(reinterpret_cast<__block_type>(__f ? __function::_Block_copy(__f) : nullptr))

if (__x.__id_ == 0)
return __y.__id_ != 0;
if (__y.__id_ == 0)
if (__x.__id_ == nullptr)
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like __x.__id_ isn't a pointer on other platforms. I guess we just have to disable the check for this file.

@ldionne
Copy link
Member

ldionne commented Aug 19, 2024

Gentle ping on this!

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
None yet
Development

Successfully merging this pull request may close these issues.

4 participants