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

TST: avoid unused variable in assert #3175

Merged
merged 1 commit into from
Apr 26, 2024
Merged

Conversation

minrk
Copy link
Contributor

@minrk minrk commented Apr 26, 2024

CHECK appears to be the way checks are done elsewhere, this was the only assert left in the file.

I assume assert is being optimized out, causing compiler error with -Werror=unused-variable. cpp tests pass during conda build with this patch.

First try at #3174 was wrong, but it didn't let me reopen that PR after I figured it out because I updated the branch while it was closed.

CHECK appears to be the way checks are done elsewhere
@jhale jhale added the backport? Suggest PR for backporting label Apr 26, 2024
@garth-wells garth-wells added the testing Test system issues label Apr 26, 2024
@garth-wells
Copy link
Member

Thanks - clearly wrong to be using assert here.

@jhale
Copy link
Member

jhale commented Apr 26, 2024

@minrk could you help us get our compile flags tightened so this doesn't happen again?

I'm surprised -Wall doesn't include this one.

https://github.com/FEniCS/dolfinx/blob/main/cpp/test/CMakeLists.txt#L51

@garth-wells garth-wells added this pull request to the merge queue Apr 26, 2024
@minrk
Copy link
Contributor Author

minrk commented Apr 26, 2024

conda-forge adds -DNDEBUG to CPPFLAGS, which strips asserts. It would be reasonable to remove -DNDEBUG from the compile args in the test directory, I would imagine, which would also allow this to pass. I don't know the best cmake incantation to remove arguments.

If you know you don't want asserts, you could add -DNDEBUG by default as well, but I'm not sure that's the right way to do it, because other asserts that aren't the only use of variables would just get silently skipped. Seems like a linter ought to be the level of avoiding asserts, but I don't know the state of linters for C++.

Merged via the queue into FEniCS:main with commit 079058c Apr 26, 2024
14 checks passed
@minrk minrk deleted the patch-1 branch April 26, 2024 10:02
garth-wells added a commit that referenced this pull request Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport? Suggest PR for backporting testing Test system issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants