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

Use proper check for non-type template parameters #2247

Merged
merged 3 commits into from
Apr 26, 2021

Conversation

alexezeder
Copy link
Contributor

@alexezeder alexezeder commented Apr 22, 2021

More info here - #2240 (comment)

Also, FMT_CONSTEXPR_CHAR_TRAITS was fixed for MSVC and two C++20 MSVC configurations were added to CI:

  • x64, Debug
  • x64, Release

@vitaut
Copy link
Contributor

vitaut commented Apr 23, 2021

I got some strange errors on CI for other non-related tests

I wonder if they can be reproduced on godbolt.

@alexezeder
Copy link
Contributor Author

I wonder if they can be reproduced on godbolt.

I don't think it's possible, because they are runtime errors, but ASAIK godbolt couldn't run executables with the MSVC toolchain.

The problem, as I see it, is that MSVC installed on CI cannot prepare the correct executable with C++20 enabled. Even core-test.exe fails with some wild error, that seems to be Windows' SIGSEGV.

I don't know what causes this error: maybe it's the specific MSVC version on CI, maybe even the latest MSVC version with C++20 enabled has this error... who knows. I'm 99.9% sure that the changed check works for Clang, GCC and MSVC, because I tested this check on Compiler Explorer separately, but I'm unable to test how does NTTP work on MSVC because of this error.

@alexezeder
Copy link
Contributor Author

alexezeder commented Apr 24, 2021

I found the cause of this problem - it's the outdated/custom GMock/GTest library. SEH is invoked by lines like this EXPECT_CALL(buffer, do_grow(124));, maybe not all lines like this invoke SEH, but some definitely do. After updating GMock/GTest locally, this problem disappeared.

But after updating GMock/GTest, I thought - why can't we just take GMock/GTest from the official repo, shrink it if needed, probably add a custom CMakeLists.txt, and we would have an easy-maintainable test library. IMHO, what we have right now is odd because it looks like besides some probably meaningful fixes that probably should also be fixed in the original repo:
db0d54f#diff-a5b1144127424777c97623a893fd7f00728a930b24d96d5c744bc1d448252980
besides fixes for compiler warnings:
466386d#diff-27f409b3775ec32883b6f38e9530ad8a1c2b342b89fff537cd61b9cd36e8b3c2
and
d00b43c#diff-27f409b3775ec32883b6f38e9530ad8a1c2b342b89fff537cd61b9cd36e8b3c2
which probably not relevant anymore because of SYSTEM includes:

target_include_directories(gmock SYSTEM PUBLIC . gmock gtest)

there are some really strange changes like:
7f723fb#diff-725d68160f11d33d5caa60bd3fd7bad7c19028b7f36aaae0bf1ff383c71d6cec
or even
58b6f8d#diff-725d68160f11d33d5caa60bd3fd7bad7c19028b7f36aaae0bf1ff383c71d6cec.

So, maybe we should use not only updated but reorganized (or should I say non-reorganized, by hand) GMock/GTest?

@vitaut
Copy link
Contributor

vitaut commented Apr 24, 2021

Yeah, we should update gtest/gmock.

@alexezeder
Copy link
Contributor Author

Yeah, we should update gtest/gmock.

But the question is how should we do this, by taking the untouched version from googletest repo or by updating the existing files?

I like the first option and can create a separate PR for this change. But maybe there are some pitfalls with that approach.

@vitaut
Copy link
Contributor

vitaut commented Apr 24, 2021

But the question is how should we do this, by taking the untouched version from googletest repo or by updating the existing files?

Taking the fused version of gtest and gmock. I tried to update them in 3f4a99b and it mostly works but requires some portability fixes for older platforms.

@alexezeder
Copy link
Contributor Author

Taking the fused version of gtest and gmock.

Ohh... that's why files look so strange, I was not aware of that script.

@vitaut
Copy link
Contributor

vitaut commented Apr 25, 2021

I've updated gtest/gmock which hopefully resolves the issue with core-test.

@alexezeder alexezeder marked this pull request as ready for review April 25, 2021 13:09
@alexezeder
Copy link
Contributor Author

I've updated gtest/gmock which hopefully resolves the issue with core-test.

With an updated GTest/GMock version, MSVC C++20 works fine 👍

Maybe it would be reasonable to exclude all GTest/GMock files from clang-format, so it won't be formatted accidentally. Of course, this is impossible without moving gmock-gtest-all.cc to some directory to exclude, but that's not a bad change actually.

@vitaut
Copy link
Contributor

vitaut commented Apr 25, 2021

Maybe it would be reasonable to exclude all GTest/GMock files from clang-format, so it won't be formatted accidentally.

That's a good idea. We could move gmock-gtest-all.cc to the gtest directory.

@vitaut
Copy link
Contributor

vitaut commented Apr 26, 2021

Can we reduce the number of build configs a bit? Let's remove Win32 C++20 configs since they don't add much value.

@alexezeder
Copy link
Contributor Author

After GTest update, I'm getting:

cd /fmt/cmake-build-debug-gcc-10-cxx20/test && /usr/bin/g++-10 -DGTEST_HAS_STD_WSTRING=1 -DGTEST_LANG_CXX11=1 -D_SILENCE_TR1_NAMESPACE_DEPRECATION_WARNING=1 -isystem /fmt/test/. -isystem /fmt/test/gmock -isystem /fmt/test/gtest -g -std=gnu++2a -o CMakeFiles/gmock.dir/gmock-gtest-all.cc.o -c /fmt/test/gmock-gtest-all.cc
In file included from /fmt/test/gmock-gtest-all.cc:38:
/fmt/test/gtest/gtest.h: In static member function ‘static constexpr bool testing::internal::MatcherBase<T>::IsInlined()’:
/fmt/test/gtest/gtest.h:6512:17: warning: ‘template<class _Tp> struct std::is_pod’ is deprecated: use is_standard_layout && is_trivial instead [-Wdeprecated-declarations]
 6512 |            std::is_pod<M>::value &&
      |                 ^~~~~~
In file included from /usr/include/c++/10/bits/move.h:57,
                 from /usr/include/c++/10/bits/stl_pair.h:59,
                 from /usr/include/c++/10/bits/stl_algobase.h:64,
                 from /usr/include/c++/10/memory:63,
                 from /fmt/test/gtest/gtest.h:57,
                 from /fmt/test/gmock-gtest-all.cc:38:
/usr/include/c++/10/type_traits:697:5: note: declared here
  697 |     is_pod
      |     ^~~~~~

because std::is_pod is deprecated in C++20. And the SYSTEM includes probably don't help here because this line:

#include "gtest/gtest.h"

includes gtest.h as a relative path, but not as a system.

@vitaut
Copy link
Contributor

vitaut commented Apr 26, 2021

How about replacing is_pod with is_trivial?

@alexezeder
Copy link
Contributor Author

How about replacing is_pod with is_trivial?

Maybe it will be better to do something like this - alexezeder@7d5d037, since we are already changing GTest library.

I was thinking about PR this change to the GTest repo, but looking at the amount of PRs there and their dates... it's probably almost impossible. Also, they specified that only GCC 5.0+ are supported, no one likes old compilers 😔

@vitaut
Copy link
Contributor

vitaut commented Apr 26, 2021

Maybe it will be better to do something like this - alexezeder/fmt@7d5d037

Sounds good.

@vitaut vitaut merged commit 77258f6 into fmtlib:master Apr 26, 2021
@vitaut
Copy link
Contributor

vitaut commented Apr 26, 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.

2 participants