search

max0x7ba / atomic_queue

C++ lockless queue.
MIT License
1.47k stars 176 forks source link

Elaborate in docs what "atomic elements" means. #41

Closed libbooze closed 2 years ago

libbooze commented 2 years ago

I found this in source, but it was not obvious from reading the docs:

assert(std::atomic<T>{NIL}.is_lock_free()); // This queue is for atomic elements only. AtomicQueue2 is for non-atomic ones.

I for one was thinking that maybe the Titself needs to be std::atomic<SomeOtherT>.

P.S. Not sure if that assertcould be static_assert, IDK if there can be a type that is_lock_free but not constructible at compile time.

max0x7ba commented 2 years ago

I for one was thinking that maybe the T itself needs to be std::atomic<SomeOtherT>.

std::atomic<T>{NIL}.is_lock_free() must be true for T to be considered atomic and lock-free. You are right that the documentation wording may be less than ideal. I am open to your pull requests.

I for one was thinking that maybe the T itself needs to be std::atomic<SomeOtherT>.

That calls for explicitly deleted queues partial specializations for std::atomic<T>, which makes good sense.

Not sure if that assert could be static_assert, IDK if there can be a type that is_lock_free but not constructible at compile time.

static_assert would be ideal for that, you are quite right. https://en.cppreference.com/w/cpp/atomic/atomic/is_lock_free is not constexpr, sadly, which precludes from using it in static_assert.

C++17 offers another option https://en.cppreference.com/w/cpp/atomic/atomic/is_always_lock_free, which I may add in the future, unless someone submits a pull request for that earlier.

As a side note, I would like to encourage you to always try out your own suggestions and submit successful ones as pull requests. Pull requests is often the best way to complain and having your complaint resolved fastest.

max0x7ba commented 2 years ago

Thank you for your pull request.

I minimized the wording, added a few more words and moved the links to the bottom, and submitted a new commit only to not have to update yours.

max0x7ba commented 2 years ago

If you are happy you may like to close this issue. Otherwise, let me know.

libbooze commented 2 years ago

I like your changes better, thank you very much for fixing this.