[libc++] Implement `thread::id` comparators as free functions

[avogelsgesang]

Summary

So far, the thread::id comparators were implemented as hidden friends.
This was non-conforming and lead to incorrectly rejected C++ code, as
can be seen in the linked Github issue.

Fixes https://github.com/llvm/llvm-project/issues/56187

[avogelsgesang]

use correct visibility macros

[Mordante]

Thanks for fixing this bug!
Can you mention this change to the API changes of the release documentation?

libcxx/include/__threading_support

619

Is there a reason to make all 6 of them friends? I would only operator== and operator< to be a friend.

libcxx/test/std/thread/thread.threads/thread.thread.class/thread.thread.id/eq.pass.cpp

36

Nice!

[avogelsgesang]

address comments

[avogelsgesang]

Can you mention this change to the API changes of the release documentation?

Afaict, there is no "API changes" in the release notes, only "ABI changes". I added the item to the "Improvements" section instead

[Mordante]

In D131430#3713407, @avogelsgesang wrote:

Can you mention this change to the API changes of the release documentation?

Afaict, there is no "API changes" in the release notes, only "ABI changes". I added the item to the "Improvements" section instead

We used to have an API section, for example https://releases.llvm.org/14.0.0/projects/libcxx/docs/ReleaseNotes.html#api-changes

Can you re-add that section and move the new documentation there?

LGTM after moving the documentation.

[avogelsgesang]

Introduce "API changes" section and move description of this change there

libcxx/docs/ReleaseNotes.rst

 -----------------------------
 
 Deprecations and Removals
 -------------------------
 
 Upcoming Deprecations and Removals
 ----------------------------------
 
+API Changes
+-----------
+- The comparison operators on ``thread::id`` are now defined as free-standing
+  functions instead of as hidden friends, in conformance with the C++ standard.
+  Also see `issue 56187 <https://github.com/llvm/llvm-project/issues/56187>`_.
+
 ABI Affecting Changes
 ---------------------
 
 Build System Changes
 --------------------

libcxx/include/__threading_support

class _LIBCPP_TEMPLATE_VIS __thread_id
     // NULL is the no-thread value on Darwin.  Someone needs to check
     // on other platforms.  We assume 0 works everywhere for now.
     __libcpp_thread_id __id_;
 
 public:
     _LIBCPP_INLINE_VISIBILITY
     __thread_id() _NOEXCEPT : __id_(0) {}
 
-    friend _LIBCPP_INLINE_VISIBILITY
-        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) return false;
-        return __libcpp_thread_id_equal(__x.__id_, __y.__id_);
-        }
-    friend _LIBCPP_INLINE_VISIBILITY
-        bool operator!=(__thread_id __x, __thread_id __y) _NOEXCEPT
-        {return !(__x == __y);}
-    friend _LIBCPP_INLINE_VISIBILITY
-        bool operator< (__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) return false;
-        return  __libcpp_thread_id_less(__x.__id_, __y.__id_);
-        }
-    friend _LIBCPP_INLINE_VISIBILITY
-        bool operator<=(__thread_id __x, __thread_id __y) _NOEXCEPT
-        {return !(__y < __x);}
-    friend _LIBCPP_INLINE_VISIBILITY
-        bool operator> (__thread_id __x, __thread_id __y) _NOEXCEPT
-        {return   __y < __x ;}
-    friend _LIBCPP_INLINE_VISIBILITY
-        bool operator>=(__thread_id __x, __thread_id __y) _NOEXCEPT
-        {return !(__x < __y);}
-
     _LIBCPP_INLINE_VISIBILITY
     void __reset() { __id_ = 0; }
 
+    friend _LIBCPP_HIDE_FROM_ABI bool operator==(__thread_id __x, __thread_id __y) _NOEXCEPT;
+    friend _LIBCPP_HIDE_FROM_ABI bool operator< (__thread_id __x, __thread_id __y) _NOEXCEPT;
+
     template<class _CharT, class _Traits>
     friend
     _LIBCPP_INLINE_VISIBILITY
     basic_ostream<_CharT, _Traits>&
     operator<<(basic_ostream<_CharT, _Traits>& __os, __thread_id __id);
 
 private:
     _LIBCPP_INLINE_VISIBILITY
     __thread_id(__libcpp_thread_id __id) : __id_(__id) {}
 
     friend __thread_id this_thread::get_id() _NOEXCEPT;
     friend class _LIBCPP_TYPE_VIS thread;
     friend struct _LIBCPP_TEMPLATE_VIS hash<__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)
+    return false;
+  return __libcpp_thread_id_equal(__x.__id_, __y.__id_);
+}
+
+inline _LIBCPP_HIDE_FROM_ABI
+bool operator!=(__thread_id __x, __thread_id __y) _NOEXCEPT {
+    return !(__x == __y);
+}
+
+inline _LIBCPP_HIDE_FROM_ABI
+bool operator<(__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)
+    return false;
+  return __libcpp_thread_id_less(__x.__id_, __y.__id_);
+}
+
+inline _LIBCPP_HIDE_FROM_ABI bool operator<=(__thread_id __x, __thread_id __y) _NOEXCEPT { return !(__y < __x); }
+inline _LIBCPP_HIDE_FROM_ABI bool operator>(__thread_id __x, __thread_id __y) _NOEXCEPT { return __y < __x; }
+inline _LIBCPP_HIDE_FROM_ABI bool operator>=(__thread_id __x, __thread_id __y) _NOEXCEPT { return !(__x < __y); }
+
 namespace this_thread
 {
 
 inline _LIBCPP_INLINE_VISIBILITY
 __thread_id
 get_id() _NOEXCEPT
 {
     return __libcpp_thread_get_current_id();

libcxx/test/std/thread/thread.threads/thread.thread.class/thread.thread.id/eq.pass.cpp

int main(int, char**)
     std::thread::id id1;
     id1 = id0;
     assert( (id1 == id0));
     assert(!(id1 != id0));
     id1 = std::this_thread::get_id();
     assert(!(id1 == id0));
     assert( (id1 != id0));
 
+    // Regression tests for https://github.com/llvm/llvm-project/issues/56187
+    // libc++ previously declared the comparison operators as hidden friends
+    // which was non-conforming.
+    assert(!std::operator==(id1, id0));
+    assert(std::operator!=(id1, id0));
+
     return 0;
 }

libcxx/test/std/thread/thread.threads/thread.thread.class/thread.thread.id/lt.pass.cpp

if (id0 < id2) {
       assert(!(id0 >  id2));
       assert(!(id0 >= id2));
     } else {
       assert(!(id0 <= id2));
       assert( (id0 >  id2));
       assert( (id0 >= id2));
     }
 
+    // Regression tests for https://github.com/llvm/llvm-project/issues/56187
+    // libc++ previously declared the comparison operators as hidden friends
+    // which was non-conforming.
+    assert(!std::operator<(id0, id1));
+    assert(std::operator<=(id0, id1));
+    assert(!std::operator>(id0, id1));
+    assert(std::operator>=(id0, id1));
+
     return 0;
 }