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

Improve PCH and enable PCH for Catch2 and tests #2894

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Improve PCH and enable PCH for Catch2 and tests #2894

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ba39ec3
Improve PCH and enable PCH for Catch2 and tests
vittorioromeo Feb 8, 2024
fe735e9
Merge branch 'master' of https://github.com/SFML/SFML into feature/pc…
vittorioromeo May 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,12 @@ set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${PROJECT_BINARY_DIR}/bin)
set_property(GLOBAL PROPERTY USE_FOLDERS ON)
set_property(GLOBAL PROPERTY PREDEFINED_TARGETS_FOLDER "CMake")

if(SFML_BUILD_TEST_SUITE)
# this definition is used in the PCH file to speed up compilation of tests
# `add_definitions` must be called before `add_subdirectory`
add_definitions(-DSFML_BUILD_TEST_SUITE)
endif()

# add the subdirectories
add_subdirectory(src/SFML)

Expand Down
2 changes: 2 additions & 0 deletions examples/joystick/Joystick.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
#include <string>
#include <unordered_map>

#include <cstdlib>


namespace
{
Expand Down
14 changes: 14 additions & 0 deletions src/SFML/PCH.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
#include <SFML/System/Vector2.hpp>

#include <algorithm>
#include <array>
#include <chrono>
#include <filesystem>
#include <iostream>
Expand All @@ -62,3 +63,16 @@
#include <cstdint>
#include <cstdlib>
#include <cstring>


////////////////////////////////////////////////////////////
// Precompiled Headers (Test Suite Only)
////////////////////////////////////////////////////////////

#ifdef SFML_BUILD_TEST_SUITE

Copy link
Member

Choose a reason for hiding this comment

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

Why not just pass the path to this header to the target_precompiled_headers call within test/CMakeLists.txt? Feels out of place to add non-SFML headers to PCH.hpp. As seen it’s causing downstream complications which I’d rather we avoid.

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean to directly pass catch_test_macros instead of PCH.hpp when building Catch?

That was my first approach, but I noticed that Catch2 shares quite a lot of headers in common with SFML (e.g. ostream, algorithm, vector, memory), so it made sense to reuse the existing PCH.hpp rather than creating a new ad-hoc one for Catch2.

The other issue is that we want catch_test_macros to also be part of SFML's own PCH because we use that PCH in our own test suite.

So keeping everything in the same PCH seemed the most reasonable choice here.

We'd still have to keep catch2/catch_test_macros in SFML's own PCH to speed up compilation of our test drivers.

Copy link
Member

Choose a reason for hiding this comment

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

Can’t you have multiple PCH sets? And then reuse the PCH set for the core library alongside the PCH for catch_test_macros?

Copy link
Member Author

Choose a reason for hiding this comment

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

Can’t you have multiple PCH sets? And then reuse the PCH set for the core library alongside the PCH for catch_test_macros?

I don't see how that solves the issue, it would limit PCH reuse unless I am mistaken...

There are basically three categories of targets:

  1. SFML library: needs PCH_STDLIB + PCH_SFML
  2. SFML tests: needs PCH_STDLIB + PCH_SFML + PCH_CATCH
  3. Catch2 library: needs PCH_STDLIB + PCH_CATCH

Right now, my PCH is PCH_STDLIB + PCH_SFML + PCH_CATCH. This allows me to reuse the same PCH for all three categories of targets.

I definitely want "SFML library" and "SFML tests" to reuse the same PCH, to avoid recompiling two different PCHs -- but that requires that PCH to have access to Catch2 headers anyway.

If we were to go with your solution, then we'd have a few PCH files:

  1. PCHStdlib.hpp
  2. PCHStdlibSFML.hpp (includes PCHStdlib.hpp)
  3. PCHStdlibCatch.hpp (includes PCHStdlib.hpp)
  4. PCHStdlibSFMLCatch.hpp (include PCHSFML.hpp and PCHCatch.hpp)
  • SFML libraries would use PCHStdlibSFML.hpp.
  • SFML tests would use PCHStdlibSFMLCatch.hpp.
  • Catch would use PCHStdlibCatch.hpp.

I would be able to use PCHStdlibSFML.hpp for SFML libraries, and use REUSE_FROM from sfml-system.

But if I had to use PCHStdlibSFMLCatch.hpp for tests I would lose the ability to REUSE_FROM from sfml-system, which is a step backwards compared to the current approach. The same also applies to SFML examples.

I don't think going down this route would be better... in the end PCHs are an optimization of build times which already heavily screws up header hygiene, I think it's acceptable to use a few target_include_directories to make everything work when PCHs are enabled.

#include <catch2/catch_test_macros.hpp>

#include <map>

#endif
15 changes: 15 additions & 0 deletions test/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,21 @@ set_target_properties(Catch2WithMain PROPERTIES EXPORT_COMPILE_COMMANDS OFF)
get_target_property(CATCH2_INCLUDE_DIRS Catch2 INTERFACE_INCLUDE_DIRECTORIES)
target_include_directories(Catch2 SYSTEM INTERFACE ${CATCH2_INCLUDE_DIRS})

# enable precompiled headers
if (SFML_ENABLE_PCH)
message(VERBOSE "enabling PCH for Catch2")
eXpl0it3r marked this conversation as resolved.
Show resolved Hide resolved
target_precompile_headers(Catch2 PRIVATE ${PROJECT_SOURCE_DIR}/src/SFML/PCH.hpp)

# necessary to find `WindowsHeader.hpp` on Windows
target_include_directories(Catch2 PRIVATE ${PROJECT_SOURCE_DIR}/src)

# necessary to find SFML headers from Catch2 through the PCH header
target_include_directories(Catch2 PRIVATE ${PROJECT_SOURCE_DIR}/include)

# necessary to find Catch2 headers from SFML through the PCH header
target_include_directories(sfml-system PRIVATE ${CATCH2_INCLUDE_DIRS})
Comment on lines +32 to +36
Copy link
Member Author

Choose a reason for hiding this comment

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

@eXpl0it3r: this seems to solve the build on Windows, if you have any suggestions on a more elegant solution I'm all ears :)

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if there isn't some include directory variable thingy from the target we can use. Maybe @ChrisThrasher has an idea.

endif()

add_library(sfml-test-main STATIC
TestUtilities/SystemUtil.hpp
TestUtilities/SystemUtil.cpp
Expand Down