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

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

Closed
BillyONeal opened this issue Jan 27, 2020 · 5 comments · Fixed by #140
Closed

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

BillyONeal opened this issue Jan 27, 2020 · 5 comments · Fixed by #140

Comments

@BillyONeal
Copy link
Contributor

P0883 marks atomic_init as deprecated, which causes cppcoro to throw a bunch of deprecation errors when used with our standard library (post microsoft/STL#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
Copy link
Contributor Author

(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
Copy link
Owner

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
Copy link
Contributor Author

BillyONeal commented Jan 28, 2020

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
Copy link
Contributor Author

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

@BillyONeal
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants