Skip to content

use target_compile_definitions for REFLECTCPP_USE_STD_EXPECTED instea… #507

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

Merged
merged 1 commit into from
Sep 30, 2025
Merged

use target_compile_definitions for REFLECTCPP_USE_STD_EXPECTED instea… #507

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ option(REFLECTCPP_USE_BUNDLED_DEPENDENCIES "Use the bundled dependencies" ON)
option(REFLECTCPP_USE_STD_EXPECTED "Use std::expected instead of the built-in Result type (requires C++-23)" OFF)

if(REFLECTCPP_USE_STD_EXPECTED)
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -DREFLECTCPP_USE_STD_EXPECTED")
if(NOT DEFINED CMAKE_CXX_STANDARD)
set(CMAKE_CXX_STANDARD 23)
endif()
Expand Down Expand Up @@ -160,6 +159,10 @@ else()
target_compile_options(reflectcpp PRIVATE $<$<CONFIG:Debug>:-Wall -Wextra>)
endif()

if(REFLECTCPP_USE_STD_EXPECTED)
target_compile_definitions(reflectcpp PUBLIC REFLECTCPP_USE_STD_EXPECTED)
endif()
Comment on lines +162 to +164
Copy link

Choose a reason for hiding this comment

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

medium

This change correctly uses target_compile_definitions, which is an improvement. However, it introduces a second if(REFLECTCPP_USE_STD_EXPECTED) block, duplicating the check from line 28. This can make the build logic harder to follow and maintain.

To consolidate this logic and make the CMake script more robust and modern, I'd suggest using target_compile_features to specify C++ standard requirements directly on the reflectcpp target. This would also provide better error messages if an incompatible C++ standard is used.

You could replace this new block and the one at lines 28-38 with a single block after add_library(reflectcpp ...):

# After add_library(reflectcpp ...)
if(REFLECTCPP_USE_STD_EXPECTED)
    target_compile_features(reflectcpp PUBLIC cxx_std_23)
    target_compile_definitions(reflectcpp PUBLIC REFLECTCPP_USE_STD_EXPECTED)
else()
    target_compile_features(reflectcpp PUBLIC cxx_std_20)
endif()

This would remove the need to manipulate the global CMAKE_CXX_STANDARD variable and centralize the feature configuration for the reflectcpp target.


set(REFLECT_CPP_SOURCES
src/reflectcpp.cpp
)
Expand Down
Loading