jbaldwin / libcoro

C++20 coroutine library
Apache License 2.0
599 stars 62 forks source link

Clean: Fix MSVC compiler warning. #192

Closed bjones1 closed 1 year ago

a858438680 commented 1 year ago

why do not change it to static assert?

bjones1 commented 1 year ago

A good point. I like that, but then it means we can't (easily) test that this works. I found a blog post on this, but it seems like a lot of work for one test.

So, @jbaldwin, what do you prefer? The static assert would be ring_buffer() { static_assert(num_elements != 0, "num_elements cannot be zero"); } plus dropping the lines 8-12 in test_ring_buffer.cpp.

jbaldwin commented 1 year ago

Static assert is probably the correct way to solve this, pushing towards compile time errors is usually better. It's simple enough too we probably don't need to test that it fails to compile in a test IMO, but won't turn down a PR that does test it.

bjones1 commented 1 year ago

Thanks @a858438680 for bringing this up! Here's the improved version per @jbaldwin's request.

bjones1 commented 1 year ago

Note that the fedora-36 failure is a download problem, not a test failure.

jbaldwin commented 1 year ago

Retriggered the fed 36 build