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

Update GTest/GMock structure, apply some fixes #2256

Merged
merged 4 commits into from
Apr 29, 2021

Conversation

alexezeder
Copy link
Contributor

@alexezeder alexezeder commented Apr 26, 2021

  • all GTest/GMock files moved to test/gtest directory
  • CMakeLists.txt created in test/gtest from CMakeLists.txt in test
  • GTest/GMock target in CMake renamed to gtest (was gmock)
  • CMake gtest target updated to export includes as "gtest/gtest.h" or "gmock/gmock.h" only
  • includes in tests updated: "gtest.h" -> "gtest/gtest.h", "gmock.h" -> "gmock/gmock.h"
  • removed duplications of target_include_directories for GTest/GMock directories (CMake manages them)
  • .clang-format file added into test/gtest directory to prevent formatting there
  • obsolete GTEST_LANG_CXX11 compile definition setting removed
  • std::is_trivially_copy_constructible for GCC 4.8 & 4.9 fixed properly

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. In general looks good but let's use the directory structure generated by the fuse script (not introduce include directories) and use gtest instead of gmock as the top-level directory since the gmock component is way less important:

gtest/
  gtest/gtest.h
  gmock/gmock.h
  gmock-gtest-all.cc

@alexezeder
Copy link
Contributor Author

use gtest instead of gmock as the top-level directory since the gmock component is way less important

Maybe I should also rename CMake target then?

@vitaut
Copy link
Contributor

vitaut commented Apr 28, 2021

Maybe I should also rename CMake target then?

Good idea.

@alexezeder
Copy link
Contributor Author

alexezeder commented Apr 28, 2021

let's use the directory structure generated by the fuse script (not introduce include directories)

gtest/
  gtest/gtest.h
  gmock/gmock.h
  gmock-gtest-all.cc

Sorry for breaking your message into pieces, but I just realized that include directory was introduced on purpose. As we saw here, this structure makes compilers use GTest/GMock includes as a relative path, but not a system, while building gmock-gtest-all.cc, because of "", not <> includes. At the same time, only the system includes allows us to get rid of potential compiler warnings. Of course, there was a problem with std::pod only, but who knows what else suspicious various compilers can find inside the GTest library.

@alexezeder
Copy link
Contributor Author

On the other hand, maybe warnings (not errors, if no other CXX_FLAGS passed to CMake) from the GTest library, while building (currently) gmock CMake target, are not so bad to change the structure...

@vitaut
Copy link
Contributor

vitaut commented Apr 28, 2021

maybe warnings (not errors, if no other CXX_FLAGS passed to CMake) from the GTest library, while building (currently) gmock CMake target, are not so bad to change the structure...

Yeah, I think warnings are OK.

…ck usages

* all GTest/GMock files moved to `test/gtest` directory
* `CMakeLists.txt` created in `test/gtest` from `CMakeLists.txt` in `test`
* GTest/GMock target in CMake renamed to `gtest` (was `gmock`)
* CMake `gtest` target updated to export includes as "gtest/gtest.h" or "gmock/gmock.h" only
* includes in tests updated: "gtest.h" -> "gtest/gtest.h", "gmock.h" -> "gmock/gmock.h"
* removed duplications of `target_include_directories` for GTest/GMock directories (CMake manages them)
…perly

`std::is_pod<T>` was deprecated in C++20

original (pre `is_pod`) error on GCC 4.8:
```
/fmt/test/gtest/gtest.h: In static member function 'static constexpr bool testing::internal::MatcherBase<T>::IsInlined()':
/fmt/test/gtest/gtest.h:6512:12: error: 'is_trivially_copy_constructible' was not declared in this scope
            std::is_trivially_copy_constructible<M>::value &&
            ^
/fmt/test/gtest/gtest.h:6512:45: error: expected primary-expression before '>' token
            std::is_trivially_copy_constructible<M>::value &&
                                                  ^
/fmt/test/gtest/gtest.h:6512:46: error: '::value' has not been declared
            std::is_trivially_copy_constructible<M>::value &&
                                                   ^
```
@alexezeder alexezeder changed the title Move gmock files, update gmock usages Update GTest/GMock structure, apply some fixes Apr 28, 2021
@alexezeder
Copy link
Contributor Author

alexezeder commented Apr 28, 2021

I updated this PR with a few additional updates like a fix for std::is_pod usage.

Also, there are 2 places I want to discuss:

  1. fmt/test/CMakeLists.txt

    Lines 45 to 50 in 342973b

    # Workaround GTest bug https://github.com/google/googletest/issues/705.
    check_cxx_compiler_flag(
    -fno-delete-null-pointer-checks HAVE_FNO_DELETE_NULL_POINTER_CHECKS)
    if (HAVE_FNO_DELETE_NULL_POINTER_CHECKS)
    target_compile_options(test-main PUBLIC -fno-delete-null-pointer-checks)
    endif ()

    maybe it should be moved to the new CMakeLists.txt file with this flag applied to gtest CMake target? Because there are some fixes for MSVC already:
    if (MSVC)
    # Disable MSVC warnings of _CRT_INSECURE_DEPRECATE functions.
    target_compile_definitions(gtest PRIVATE _CRT_SECURE_NO_WARNINGS)
    if (CMAKE_CXX_COMPILER_ID MATCHES "Clang")
    # Disable MSVC warnings of POSIX functions.
    target_compile_options(gtest PUBLIC -Wno-deprecated-declarations)
    endif ()
    endif ()
  2. fmt/test/CMakeLists.txt

    Lines 52 to 56 in 342973b

    # Use less strict pedantic flags for the tests because GMock doesn't compile
    # cleanly with -pedantic and -std=c++98.
    if (CMAKE_COMPILER_IS_GNUCXX OR (CMAKE_CXX_COMPILER_ID MATCHES "Clang"))
    #set(PEDANTIC_COMPILE_FLAGS -Wall -Wextra -Wno-long-long -Wno-variadic-macros)
    endif ()

    maybe it should be removed? 😄

@alexezeder alexezeder requested a review from vitaut April 29, 2021 07:48
@vitaut vitaut merged commit fd43e4d into fmtlib:master Apr 29, 2021
@vitaut
Copy link
Contributor

vitaut commented Apr 29, 2021

Awesome, thanks!

@alexezeder
Copy link
Contributor Author

Great, but what do you think about #2256 (comment)? 🙂

@vitaut
Copy link
Contributor

vitaut commented Apr 29, 2021

what do you think about #2256 (comment)?

Both suggestions look reasonable to me.

@alexezeder alexezeder mentioned this pull request Dec 13, 2021
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