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.

Sign up for GitHub

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

Jump to bottom

Remove incorrect C++20 check from test/CMakeLists.txt #2663

Merged
merged 1 commit into from
Dec 23, 2021
Merged

Remove incorrect C++20 check from test/CMakeLists.txt #2663

merged 1 commit into from
Dec 23, 2021

Conversation

alexezeder
Copy link
Contributor

@alexezeder alexezeder commented Dec 18, 2021 •
edited
Loading

This check disables entire branch of tests declaration unconditionally because CXX_STANDARD is not defined there. But even if we use CMAKE_CXX_STANDARD there, these tests should work for standards ≥20 too, IMHO.

Here are the tests:

  • std-format-test (now removed)
  • noexception-test
  • nolocale-test
  • compile-error-test

std-format-test does not compile after enabling it, since it's either outdated or should still be disabled. @vitaut, could you explain what should be done with it?

@alexezeder alexezeder force-pushed the reenable_disabled_tests branch from bf9f553 to f5070a6 Compare December 18, 2021 16:57
@alexezeder alexezeder changed the title Remove C++20 check from test/CMakeLists.txt Remove incorrect C++20 check from test/CMakeLists.txt Dec 18, 2021
vitaut
vitaut requested changes Dec 19, 2021
View reviewed changes
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.

std-format-test is incompatible with C++20 so the check should be preserved but possibly moved to a place where it only applies to this test.

@alexezeder
Copy link
Contributor Author

I've disabled std-format-test with standards ≥ C++20, but there are still errors in format header.

@alexezeder alexezeder requested a review from vitaut December 20, 2021 17:54
@alexezeder
Copy link
Contributor Author

@vitaut, I suggest merging this PR to some temporary branch, where you can apply needed changes for std-format-test since I'm not sure that I can adapt it to the latest version of the library. Of course, you could guide me here by discussions, but it would be much faster for you to fix the remaining problems. What do you think about that?
Or we can simply disable std-format-test for now. In this case, I will create an issue to enable it again.

@alexezeder alexezeder mentioned this pull request Dec 20, 2021
Merged
@vitaut
Copy link
Contributor

vitaut commented Dec 23, 2021 •
edited
Loading

I removed std-format-test since the API it has been testing has already been merged into the standard. Please rebase.

@alexezeder
remove incorrect C++20 check from test/CMakeLists.txt
d324b4b 
It disables entire branch of tests declaration unconditionally because CXX_STANDARD
is not defined there. But even we use CMAKE_CXX_STANDARD here, these tests should
not be disabled with standard >= C++20.
@alexezeder alexezeder force-pushed the reenable_disabled_tests branch from f8250b2 to d324b4b Compare December 23, 2021 20:09
@alexezeder
Copy link
Contributor Author

Rebased.

@vitaut vitaut merged commit 817788f into fmtlib:master Dec 23, 2021
@vitaut
Copy link
Contributor

vitaut commented Dec 23, 2021

Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vitaut vitaut vitaut approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@alexezeder @vitaut