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

Skip a bunch of work for tests that we don't actually want. #13019

Merged
merged 2 commits into from Dec 7, 2021

Conversation

bangerth
Copy link
Member

When setting TEST_PICKUP_REGEX, the path through the macro that is used to construct the list of tests iterates over all tests, checks the regex, checks a bunch of other things, and at the very end decides whether we actually want that test. That is a lot of work if a test doesn't match the regex -- what we should really be doing in that case is a quick exit: just check the next test.

This patch implements that: Instead of setting a flag about whether the test should be defined or not and then keep going with more work, we just call cmake's CONTINUE and skip to the next test.

There are two problems:

  • CONTINUE requires cmake 3.2, but we currently only require 3.1. That might be an acceptable thing to require these days.
  • This patch doesn't actually work and instead produces a lot of CMake warnings of the kind
CMake Error at /home/fac/g/bangerth/p/deal.II/1/build-strange/share/deal.II/macros/macro_deal_ii_add_test.cmake:351 (ADD_TEST):
  ADD_TEST given test NAME "multigrid/transfer_04b.mpirun=2.release" which
  already exists in this directory.
Call Stack (most recent call first):
  /home/fac/g/bangerth/p/deal.II/1/build-strange/share/deal.II/macros/macro_deal_ii_pickup_tests.cmake:311 (DEAL_II_ADD_TEST)
  CMakeLists.txt:4 (DEAL_II_PICKUP_TESTS)

I fail to see where that is from -- some other eyes might be quite useful. I thought I'd post the patch anyway to get the idea out there.

@tamiko?

@bangerth
Copy link
Member Author

bangerth commented Dec 6, 2021

@tamiko ?

@tamiko
Copy link
Member

tamiko commented Dec 6, 2021

@bangerth On it.

@tamiko
Copy link
Member

tamiko commented Dec 6, 2021

@bangerth CONTINUE() was in the wrong for loop ^^

@tamiko
Copy link
Member

tamiko commented Dec 6, 2021

/rebuild

@@ -298,11 +294,15 @@ Comparison operator \"=\" expected for boolean match.\n"
"${DEAL_II_${_feature}_VERSION}" VERSION_LESS "${_version}" ) OR
( "${_operator}" STREQUAL ".leq." AND
"${DEAL_II_${_feature}_VERSION}" VERSION_GREATER "${_version}" ) )
CONTINUE() # next test
SET(_skip_test TRUE)
ENDIF()
ENDIF()
ENDFOREACH()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bangerth You missed this sneaky ENDFOREACH() here...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, and there I have been railing for many years that break and continue in C/C++ are dangerous because it is so easy to get things wrong when there are nested levels of loops -- and because one can so easily change the meaning of a break/continue when refactoring and introducing or reducing the number of nested loops. Now I'm stepping into that very same trap.

I so wished that the authors of C back in the 1960s had only allowed break and continue with a label that identifies the loop that we're working on...

@tamiko
Copy link
Member

tamiko commented Dec 7, 2021

I verified that after this changeset we still configure the same number of tests as on master.

@bangerth
Copy link
Member Author

bangerth commented Dec 7, 2021

Nice, thanks for fixing my patch! Let's hope this works.

@tamiko tamiko merged commit dfe815a into dealii:master Dec 7, 2021
@bangerth bangerth deleted the setup branch December 7, 2021 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants