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

More compile-time tests #2648

Merged
merged 4 commits into from
Dec 28, 2021
Merged

Conversation

alexezeder
Copy link
Contributor

@alexezeder alexezeder commented Dec 13, 2021

Depends on #2663.

Originated from the idea that the more compile-time checks, the better. compile-error-test test suite allows to check that newly-introduced compile-time checks (C++20) fail successfully. However, there are some nuances.

  • compile-error-test implementation changed to eliminate usages of header-only library there. It gives a bit faster compilation times (test run times):

    current version version from this PR
    one test 0s 750ms 2s 100ms
    as it is right now 4s 850ms 4s 550ms
    doubled 12s 810ms 8s 500ms
    • one test - simply one expect_compile(""),
    • as it is right now - seven expect_compile*(...),
    • doubled - additional reverse check added for every existing check, except UDL, 13 checks in total.

    Timings received on my PC, they are not very consistent, but they are within 1 second range for sure. The improvement for doubled amount of checks is not something very impressive, but it allows to add more checks without spending too much time on this test.

  • Reverse checks introduced to make sure that error tests do not fail because they become outdated. I may be wrong, but currently, wide-narrow checks fail to compile not because of these cases somehow handled, but because xchar.h header is not included. So, to be sure that these checks work, we have to add reverse check for them.

This is a draft because there are no additional checks introduced (for format string checks, for example), except for reverse checks.

Copy link
Contributor

@vitaut vitaut left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. I left some comments inline and as for std-format-test it's mostly for testing the wording of the Text Formatting paper and there is no reason to enable it in C++20.

test/compile-error-test/CMakeLists.txt Outdated Show resolved Hide resolved
test/compile-error-test/CMakeLists.txt Show resolved Hide resolved
test/compile-error-test/CMakeLists.txt Outdated Show resolved Hide resolved
test/compile-error-test/CMakeLists.txt Outdated Show resolved Hide resolved
test/compile-error-test/CMakeLists.txt Outdated Show resolved Hide resolved
test/compile-error-test/CMakeLists.txt Outdated Show resolved Hide resolved
test/compile-error-test/CMakeLists.txt.in Outdated Show resolved Hide resolved
@alexezeder
Copy link
Contributor Author

Also, I think we have to disable compile-error-test on CI with Windows, since other CMake-based tests are disabled:

fmt/test/CMakeLists.txt

Lines 183 to 184 in c5aafd8

# These tests are disabled on Windows because they take too long.
if (FMT_PEDANTIC AND NOT WIN32)

and this test takes tooooo much time to complete.

Btw, why disable slow tests not only for CI but also for local runs?

@vitaut
Copy link
Contributor

vitaut commented Dec 23, 2021

I think we have to disable compile-error-test on CI with Window

Yes, these tests are way too slow and not particularly system-specific.

why disable slow tests not only for CI but also for local runs?

Slow tests are opt in to get decent developer experience by default.

@alexezeder alexezeder force-pushed the more_compile_time_tests branch 3 times, most recently from 2bcee2c to 24c9fc3 Compare December 23, 2021 21:06
@alexezeder
Copy link
Contributor Author

alexezeder commented Dec 23, 2021

I added one more test for format string check at compile time introduced in 8.0. Not sure that I will come up with some other tests, but at least that's more tests. 😏

I'm a bit concerned about the compile-error-test execution time on CI, it almost doubles the old timings. Tomorrow (or a bit later), I will compare old and new implementations again, but this time using CI runners to be sure.

@alexezeder
Copy link
Contributor Author

alexezeder commented Dec 25, 2021

I compared old and new implementations on CI (with ubuntu-20.04 and GCC-11 in C++20 mode) and my PC (to be sure) with all erroring and non-erroring tests already presented. While preparing this comparison, I created another implementation based on PR's one, and it's even faster. Here are the results:

PR single compile test
several compile error tests
configure_file(...)
single compile test
several compile error tests
file(WRITE ...)
old implementation
1st run (x5) 83s 58s 55s 145s
2nd run (x5) 89s 54s 73s 124s
3rd run (x5) 83s 53s 65s 160s
4th run (x5) 77s 52s 64s 159s
5th run (x5) 109s 62s 51s 129s
my PC (x1) 9.94s 6.70 s 6.78 s 15.18 s
  • PR version - non-erroring tests treated the same way as erroring ones. So there are several targets for each non-erroring test. Because of that, we can specify headers we want to use for each test: core.h, format.h, xchar.h, etc. Each test links to static {fmt} library, so no additional inclusion of format-inl.h performed.
  • A single non-erroring and several erroring tests with configure_file(...) for files generation. All non-erroring tests merge into one single target since all tests should compile without errors. Headers specifiers become unnecessary since the merge result should have all headers possible to compile code fragments from all tests. This is true also for erroring tests - they should have the same headers as the merged non-erroring test. Maybe all these header specifiers are unnecessary because this is the fastest version. 😏
  • A single non-erroring and several erroring tests with file(WRITE ...) for files generation. Everything is the same as in the previous version except file generation, so no *.in files are used. A fraction of seconds slower than the previous one.
  • Old implementation based on check_cxx_source_compiles(...). Uses header-only library without hassling with additional CMake project. The slowest version.

@vitaut, I think since you already suggested making files content inline and the second and third implementations are faster, I should clean the third one a bit and update this PR with it, right?

@vitaut
Copy link
Contributor

vitaut commented Dec 25, 2021

Thanks for comprehensive testing. Merging the non-error cases is a good idea.

I think since you already suggested making files content inline and the second and third implementations are faster, I should clean the third one a bit and update this PR with it, right?

Yes, please.

@alexezeder alexezeder force-pushed the more_compile_time_tests branch 3 times, most recently from 4b4ad87 to ba2b52f Compare December 26, 2021 18:12
@alexezeder alexezeder marked this pull request as ready for review December 26, 2021 18:12
@alexezeder
Copy link
Contributor Author

And new implementation is here. Still the slowest test, but at least it works now! 😉

@alexezeder
Copy link
Contributor Author

Another test for compile-time checks of format string added. It makes sure that the incorrect name of arguments produces a compile-time error with "name"_a literal.

test/compile-error-test/CMakeLists.txt Outdated Show resolved Hide resolved
test/compile-error-test/CMakeLists.txt Outdated Show resolved Hide resolved
test/compile-error-test/CMakeLists.txt Show resolved Hide resolved
test/compile-error-test/CMakeLists.txt Outdated Show resolved Hide resolved
test/compile-error-test/CMakeLists.txt Show resolved Hide resolved
since it takes too much time to complete, similar to other tests with additional
CMake invocation
to make sure that error tests do not fail because they become outdated
@vitaut vitaut merged commit c7f8818 into fmtlib:master Dec 28, 2021
@vitaut
Copy link
Contributor

vitaut commented Dec 28, 2021

Thank you!

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