max0x7ba / atomic_queue

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

Please document the meaning of the NIL template parameter clearly #46

Closed JanosGit closed 2 years ago

JanosGit commented 2 years ago

I tried out the atomic_queue today in the context of a bigger project. The first local tests where promising, but then some of our CI tests failed. It took me a few hours to finally figure out that the reason was, that under some circumstances a default constructed element was pushed into the queue which then compared to NIL. I wasn't aware of the meaning of that third template argument and only found a description of it in an old issue.

I was about to revert all the changes and use another queue implementation because of this. Now after setting it to a different value, everything works fine. A small hint in the readme, explaining this important parameter or some documentation in the code would have saved me some hours of working time. Could you please document that somewhere?

max0x7ba commented 2 years ago

It took me a few hours to finally figure out that the reason was, that under some circumstances a default constructed element was pushed into the queue which then compared to NIL.

I wonder why this assert did not prevent from pushing NIL elements into the queue?

A small hint in the readme, explaining this important parameter or some documentation in the code would have saved me some hours of working time. Could you please document that somewhere?

That makes good sense.

JanosGit commented 2 years ago

I wonder why this assert did not prevent from pushing NIL elements into the queue?

Well, it finally was the hint for me to figure out the source of the error. However, my local tests with a debug build never triggered that case while the tests run on the CI where built as release and therefore did obviously not hit the assert. Instead I noticed a test case that was stuck and cancelled by a timeout at some point, and I intuitively didn't start searching for the error there ;) But even when I started to realize that the assert was hit I had to do some research to get a clear answer of the meaning of that parameter.

Besides that, this is some rock solid queue implementation, so thank you for sharing it with the community :)

max0x7ba commented 2 years ago

I had to do some research to get a clear answer of the meaning of that parameter.

Fair enough. NIL has been documented.

Besides that, this is some rock solid queue implementation, so thank you for sharing it with the community

Thank you, you made my day.