Re: [PATCH] Add support for C++2a stop_token

[Jonathan Wakely <jwakely at redhat dot com>]

On 23/10/19 12:50 -0700, Thomas Rodgers wrote:
> Thomas Rodgers writes:
>
> Let's try this again.

That's better, thanks :-)

> +	* include/Makefile.am: Add <stop_token> header.
> +        * include/Makefile.in: Regenerate.
> +	* include/std/stop_token: New file.
> +	* include/std/version (__cpp_lib_jthread): New value.
> +	* testsuite/30_threads/stop_token/1.cc: New test.
> +	* testsuite/30_threads/stop_token/2.cc: New test.
> +	* testsuite/30_threads/stop_token/stop_token.cc: New test.

ChangeLog entries should be provided separately from the patch (e.g.
inline in the makefile, or as a separte attachment) because they never
apply cleanly.

Also it looks like you have a mixture of tabs and space there, it
should be only tabs.

> diff --git a/libstdc++-v3/include/Makefile.in b/libstdc++-v3/include/Makefile.in
> index cd1e9df5482..9b4ab670315 100644
> --- a/libstdc++-v3/include/Makefile.in
> +++ b/libstdc++-v3/include/Makefile.in
> @@ -416,6 +416,7 @@ std_headers = \
> 	${std_srcdir}/sstream \
> 	${std_srcdir}/stack \
> 	${std_srcdir}/stdexcept \
> +	${std_srcdir}/stop_token \
> 	${std_srcdir}/streambuf \
> 	${std_srcdir}/string \
> 	${std_srcdir}/string_view \

Generated files like Makefile.in don't need to be in the patch. I use
a git alias called 'filter' to filter them out:

!f(){ git $@ | filterdiff -x '*/ChangeLog' -x '*/Makefile.in' -x '*/configure' -x '*/config.h.in' -x '*/doc/html/*' | sed -e '/^diff.*ChangeLog/{N;d}' ; }; f

And then I create a patch with this alias:

!f(){ git filter show --diff-algorithm=histogram ${*:-HEAD} > /dev/shm/patch.txt; }; f


> diff --git a/libstdc++-v3/include/std/stop_token b/libstdc++-v3/include/std/stop_token
> new file mode 100644
> index 00000000000..b3655b85eae
> --- /dev/null
> +++ b/libstdc++-v3/include/std/stop_token
> @@ -0,0 +1,338 @@
> +// <stop_token> -*- C++ -*-
> +
> +// Copyright (C) 2008-2019 Free Software Foundation, Inc.

The copyright date should be just 2019.

> +//
> +// This file is part of the GNU ISO C++ Library.  This library is free
> +// software; you can redistribute it and/or modify it under the
> +// terms of the GNU General Public License as published by the
> +// Free Software Foundation; either version 3, or (at your option)
> +// any later version.
> +
> +// This library is distributed in the hope that it will be useful,
> +// but WITHOUT ANY WARRANTY; without even the implied warranty of
> +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +// GNU General Public License for more details.
> +
> +// Under Section 7 of GPL version 3, you are granted additional
> +// permissions described in the GCC Runtime Library Exception, version
> +// 3.1, as published by the Free Software Foundation.
> +
> +// You should have received a copy of the GNU General Public License and
> +// a copy of the GCC Runtime Library Exception along with this program;
> +// see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
> +// <http://www.gnu.org/licenses/>.
> +
> +/** @file include/stop_token
> + *  This is a Standard C++ Library header.
> + */
> +
> +#ifndef _GLIBCXX_STOP_TOKEN
> +#define _GLIBCXX_STOP_TOKEN
> +

Please add:

#if __cplusplus > 201703L

here (and the corresponding #endif before the final #endif) so that
this file is empty when included pre-C++20.

> +#include <type_traits>
> +#include <memory>
> +#include <mutex>
> +#include <atomic>
> +
> +#define __cpp_lib_jthread 201907L
> +
> +namespace std _GLIBCXX_VISIBILITY(default)
> +{
> +  _GLIBCXX_BEGIN_NAMESPACE_VERSION

These BEGIN/END macros should not be indented (which has to be done
manually as editors always want to indent the content following the
opening brace of the namespace).

> +
> +  class stop_source;
> +  template<class _Callback>

s/class/typename/

> +  class stop_callback;
> +
> +  struct nostopstate_t { explicit nostopstate_t() = default; };
> +  inline constexpr nostopstate_t nostopstate();
> +
> +  class stop_token {

The opening brace should be on the next line please, in the same
column as "class".
(Same comment for all classes and structs in the patch).


> +  public:
> +    stop_token() noexcept = default;
> +
> +    stop_token(const stop_token& __other) noexcept
> +      : _M_state(__other._M_state)
> +    { }
> +
> +    stop_token(stop_token&& __other) noexcept
> +      : _M_state(std::move(__other._M_state))
> +    { }
> +
> +    ~stop_token() = default;
> +
> +    stop_token&
> +    operator=(const stop_token& __rhs) noexcept {
> +      _M_state = __rhs._M_state;
> +      return *this;
> +    }
> +
> +    stop_token&
> +    operator=(stop_token&& __rhs) noexcept {
> +      std::swap(_M_state, __rhs._M_state);
> +      return *this;
> +    }

This doesn't leave the RHS empty as required.

I think the copy/move constructors and copy/move assignment operators
can all be defined as = default and that will do the right thing.

> +
> +    [[nodiscard]]
> +    bool
> +    stop_possible() const noexcept
> +    {
> +      return static_cast<bool>(_M_state);
> +    }
> +
> +    [[nodiscard]]
> +    bool
> +    stop_requested() const noexcept
> +    {
> +      return stop_possible() && _M_state->_M_stop_requested();
> +    }
> +
> +  private:
> +    friend stop_source;
> +    template<typename _Callback>
> +    friend class stop_callback;
> +
> +    struct _Stop_cb {
> +      void(*_M_callback)(_Stop_cb*);
> +      _Stop_cb* _M_prev = nullptr;
> +      _Stop_cb* _M_next = nullptr;
> +
> +      template<typename _CB>

Please Use _Cb so it's not all uppercase (which is more likely to
conflict with libc macros).

> +      _Stop_cb(_CB&& __cb)
> +        : _M_callback(std::move(__cb))
> +      { }
> +
> +      static void
> +      _S_execute(_Stop_cb* __cb) noexcept
> +      {
> +        __cb->_M_callback(__cb);
> +      }
> +    };
> +
> +    struct _Stop_state_t {
> +      std::atomic<bool> _M_stopped;
> +      std::mutex _M_mtx;
> +      _Stop_cb* _M_head = nullptr;
> +
> +      _Stop_state_t()
> +        : _M_stopped{false}
> +      { }
> +
> +      bool
> +      _M_stop_requested()
> +      {
> +        return _M_stopped;
> +      }
> +
> +      bool
> +      _M_request_stop()
> +      {
> +        bool __stopped = false;
> +        if (_M_stopped.compare_exchange_strong(__stopped, true))
> +          {
> +            std::unique_lock<std::mutex> __lck{_M_mtx};
> +            while (auto __p = _M_head)
> +              {
> +                _M_head = __p->_M_next;
> +                _Stop_cb::_S_execute(__p);
> +              }
> +            return true;
> +          }
> +        return false;
> +      }
> +
> +      bool
> +      _M_register_callback(_Stop_cb* __cb)
> +      {
> +        std::unique_lock<std::mutex> __lck{_M_mtx};
> +        if (_M_stopped)
> +          return false;
> +
> +        __cb->_M_next = _M_head;
> +        _M_head->_M_prev = __cb;
> +        _M_head = __cb;
> +        return true;
> +      }
> +
> +      void
> +      _M_remove_callback(_Stop_cb* __cb)
> +      {
> +        std::unique_lock<std::mutex> __lck{_M_mtx};
> +        if (__cb == _M_head)
> +          {
> +            _M_head = _M_head->_M_next;
> +            _M_head->_M_prev = nullptr;
> +          }
> +        else
> +          {
> +            __cb->_M_prev->_M_next = __cb->_M_next;
> +            if (__cb->_M_next)
> +              {
> +                __cb->_M_next->_M_prev = __cb->_M_prev;
> +              }
> +          }
> +      }
> +    };
> +
> +    using _Stop_state = std::shared_ptr<_Stop_state_t>;
> +    _Stop_state _M_state;
> +
> +    explicit stop_token(_Stop_state __state)
> +      : _M_state{std::move(__state)}
> +    { }
> +  };
> +
> +  class stop_source {
> +    using _Stop_state_t = stop_token::_Stop_state_t;
> +    using _Stop_state = stop_token::_Stop_state;
> +
> +  public:
> +    stop_source()
> +      : _M_state(std::make_shared<_Stop_state_t>())
> +    { }
> +
> +    explicit stop_source(std::nostopstate_t) noexcept
> +    { }
> +
> +    stop_source(const stop_source& __other) noexcept
> +      : _M_state(__other._M_state)
> +    { }
> +
> +    stop_source(stop_source&& __other) noexcept
> +      : _M_state(std::move(__other._M_state))
> +    { }
> +
> +    stop_source&
> +    operator=(const stop_source& __rhs) noexcept
> +    {
> +      if (_M_state != __rhs._M_state)
> +        _M_state = __rhs._M_state;
> +      return *this;
> +    }
> +
> +    stop_source&
> +    operator=(stop_source&& __rhs) noexcept
> +    {
> +      std::swap(_M_state, __rhs._M_state);
> +      return *this;
> +    }

These can all be defaulted too.

> +    [[nodiscard]]
> +    bool
> +    stop_possible() const noexcept
> +    {
> +      return static_cast<bool>(_M_state);
> +    }
> +
> +    [[nodiscard]]
> +    bool
> +    stop_requested() const noexcept
> +    {
> +      return stop_possible() && _M_state->_M_stop_requested();
> +    }
> +
> +    bool
> +    request_stop() const noexcept
> +    {
> +      if (stop_possible())
> +        return _M_state->_M_request_stop();
> +      return false;
> +    }
> +
> +    [[nodiscard]]
> +    stop_token
> +    get_token() const noexcept
> +    {
> +      return stop_token{_M_state};
> +    }
> +
> +    void
> +    swap(stop_source& __other) noexcept
> +    {
> +      std::swap(_M_state, __other._M_state);
> +    }
> +
> +    [[nodiscard]]
> +    friend bool
> +    operator==(const stop_source& __a, const stop_source& __b) noexcept
> +    {
> +      return __a._M_state == __b._M_state;
> +    }
> +
> +    [[nodiscard]]
> +    friend bool
> +    operator!=(const stop_source& __a, const stop_source& __b) noexcept
> +    {
> +      return __a._M_state != __b._M_state;
> +    }
> +
> +  private:
> +    _Stop_state _M_state;
> +  };
> +
> +  template<typename _Callback>
> +    class [[nodiscard]] stop_callback
> +      : private stop_token::_Stop_cb {

Opening brace on the next line, in the same column as "class".

> +    using _Stop_cb = stop_token::_Stop_cb;

And then everything in the class body should be indented by another
two columns.

> +    using _Stop_state = stop_token::_Stop_state;
> +  public:
> +    using callback_type = _Callback;
> +
> +    template<typename _CB,

Change to _Cb here too.