lewissbaker / cppcoro

A library of C++ coroutine abstractions for the coroutines TS
MIT License
3.43k stars 472 forks source link

atomic_init calls throw deprecation warnings after acceptance of C++20 P0883 #139

Closed BillyONeal closed 4 years ago

BillyONeal commented 4 years ago

P0883 marks atomic_init as deprecated, which causes cppcoro to throw a bunch of deprecation errors when used with our standard library (post https://github.com/microsoft/STL/pull/390 ).

Should the places calling atomic_init be changed to do something like:

// earlier
#ifdef __cpp_lib_atomic_value_initialization
template<class T, class Args>
void true_placement_new(T* ptr, Args&&... args) { // do you already have a function for this?
    ::new(static_cast<void*>(ptr)) T(std::forward<Args>(args)...);
}
#endif // __cpp_lib_atomic_value_initialization

// in multi_producer_sequencer.hpp:
SEQUENCE seq = initialSequence - (bufferSize - 1);
do
{
#ifdef __cpp_lib_atomic_value_initialization
    true_placement_new(&m_published[seq & m_sequenceMask], seq);
#else
    std::atomic_init(&m_published[seq & m_sequenceMask], seq);
#endif // __cpp_lib_atomic_value_initialization
} while (seq++ != initialSequence);

instead?

BillyONeal commented 4 years ago

(I'm willing to submit a PR fixing this but any solution I can think of is invasive enough that I want to ask what you want first)

lewissbaker commented 4 years ago

I'm not sure that placement new is the correct approach for the multi_producer_sequencer call-site as the call to std::make_unique<std::atomic<SEQUENCE>[]>(bufferSize) above should have already created the atomic objects and called the default constructors. We just need to assign the atomic objects values.

I guess the cheapest way to initialize the values now is by calling .store(seq, std::memory_order_relaxed)?

I think the use of std::atomic_init() in cancellation_registration_list_chunk::allocate() should be ok to unconditionally be converted to placement new since it was previously uninitialized memory.

I should really replace cancellation_token with stop_token at some stage anyway.

BillyONeal commented 4 years ago

Ah yes, I missed the make_unique.

For the cancellation_token ones is there a specific reason it's calling malloc rather than new? It looks like new would just do the right thing.

BillyONeal commented 4 years ago

Oh, because it's using an array of size 1 like a FAM. Disregard!

BillyONeal commented 4 years ago

It looks like the placement new can be used unconditionally because C++11 even has the constructor taking a T.