Skip to content

Conversation

bjones1
Copy link
Contributor

@bjones1 bjones1 commented Oct 18, 2023

No description provided.

@a858438680
Copy link
Contributor

why do not change it to static assert?

@bjones1
Copy link
Contributor Author

bjones1 commented Oct 18, 2023

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.

@bjones1 bjones1 marked this pull request as ready for review October 18, 2023 19:57
@jbaldwin
Copy link
Owner

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

bjones1 commented Oct 22, 2023

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

@bjones1
Copy link
Contributor Author

bjones1 commented Oct 22, 2023

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

@jbaldwin
Copy link
Owner

Retriggered the fed 36 build

@jbaldwin jbaldwin merged commit f6096a3 into jbaldwin:main Oct 22, 2023
@bjones1 bjones1 deleted the fix-warning branch October 22, 2023 22:10
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 this pull request may close these issues.

3 participants