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

Use native c++ module support from CMake #3991

Merged
merged 4 commits into from
Jun 6, 2024
Merged

Use native c++ module support from CMake #3991

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b83b108
Use native c++ module support from CMake
yujincheng08 Jun 4, 2024
d728797
Check if the generator is ninja
yujincheng08 Jun 4, 2024
6d73898
Fix undefined reference when compiling with FMT_STATIC_THOUSANDS_SEPA…
yujincheng08 Jun 4, 2024
ca5915a
Reverting check to make shorter branch comes first
yujincheng08 Jun 6, 2024
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
90 changes: 47 additions & 43 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
cmake_minimum_required(VERSION 3.8...3.26)
cmake_minimum_required(VERSION 3.8...3.28)

# Fallback for using newer policies on CMake <3.12.
if (${CMAKE_VERSION} VERSION_LESS 3.12)
Expand Down Expand Up @@ -62,48 +62,52 @@ function(add_module_library name)
target_compile_options(${name} PUBLIC -fmodules-ts)
endif ()

# `std` is affected by CMake options and may be higher than C++20.
get_target_property(std ${name} CXX_STANDARD)

if (CMAKE_CXX_COMPILER_ID MATCHES "Clang")
set(pcms)
foreach (src ${sources})
get_filename_component(pcm ${src} NAME_WE)
set(pcm ${pcm}.pcm)

# Propagate -fmodule-file=*.pcm to targets that link with this library.
target_compile_options(
${name} PUBLIC -fmodule-file=${CMAKE_CURRENT_BINARY_DIR}/${pcm})

# Use an absolute path to prevent target_link_libraries prepending -l
# to it.
set(pcms ${pcms} ${CMAKE_CURRENT_BINARY_DIR}/${pcm})
add_custom_command(
OUTPUT ${pcm}
COMMAND ${CMAKE_CXX_COMPILER}
-std=c++${std} -x c++-module --precompile -c
-o ${pcm} ${CMAKE_CURRENT_SOURCE_DIR}/${src}
"-I$<JOIN:$<TARGET_PROPERTY:${name},INCLUDE_DIRECTORIES>,;-I>"
# Required by the -I generator expression above.
COMMAND_EXPAND_LISTS
DEPENDS ${src})
endforeach ()

# Add .pcm files as sources to make sure they are built before the library.
set(sources)
foreach (pcm ${pcms})
get_filename_component(pcm_we ${pcm} NAME_WE)
set(obj ${pcm_we}.o)
# Use an absolute path to prevent target_link_libraries prepending -l.
set(sources ${sources} ${pcm} ${CMAKE_CURRENT_BINARY_DIR}/${obj})
add_custom_command(
OUTPUT ${obj}
COMMAND ${CMAKE_CXX_COMPILER} $<TARGET_PROPERTY:${name},COMPILE_OPTIONS>
-c -o ${obj} ${pcm}
DEPENDS ${pcm})
endforeach ()
endif ()
target_sources(${name} PRIVATE ${sources})
if (CMAKE_VERSION VERSION_GREATER_EQUAL 3.28 AND CMAKE_GENERATOR STREQUAL "Ninja")
target_sources(${name} PUBLIC FILE_SET CXX_MODULES FILES ${sources})
else()
# `std` is affected by CMake options and may be higher than C++20.
get_target_property(std ${name} CXX_STANDARD)

if (CMAKE_CXX_COMPILER_ID MATCHES "Clang")
set(pcms)
foreach (src ${sources})
get_filename_component(pcm ${src} NAME_WE)
set(pcm ${pcm}.pcm)

# Propagate -fmodule-file=*.pcm to targets that link with this library.
target_compile_options(
${name} PUBLIC -fmodule-file=${CMAKE_CURRENT_BINARY_DIR}/${pcm})

# Use an absolute path to prevent target_link_libraries prepending -l
# to it.
set(pcms ${pcms} ${CMAKE_CURRENT_BINARY_DIR}/${pcm})
add_custom_command(
OUTPUT ${pcm}
COMMAND ${CMAKE_CXX_COMPILER}
-std=c++${std} -x c++-module --precompile -c
-o ${pcm} ${CMAKE_CURRENT_SOURCE_DIR}/${src}
"-I$<JOIN:$<TARGET_PROPERTY:${name},INCLUDE_DIRECTORIES>,;-I>"
# Required by the -I generator expression above.
COMMAND_EXPAND_LISTS
DEPENDS ${src})
endforeach ()

# Add .pcm files as sources to make sure they are built before the library.
set(sources)
foreach (pcm ${pcms})
get_filename_component(pcm_we ${pcm} NAME_WE)
set(obj ${pcm_we}.o)
# Use an absolute path to prevent target_link_libraries prepending -l.
set(sources ${sources} ${pcm} ${CMAKE_CURRENT_BINARY_DIR}/${obj})
add_custom_command(
OUTPUT ${obj}
COMMAND ${CMAKE_CXX_COMPILER} $<TARGET_PROPERTY:${name},COMPILE_OPTIONS>
-c -o ${obj} ${pcm}
DEPENDS ${pcm})
endforeach ()
endif ()
target_sources(${name} PRIVATE ${sources})
endif()
endfunction()

include(CMakeParseArguments)
Expand Down
2 changes: 2 additions & 0 deletions include/fmt/chrono.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1780,8 +1780,10 @@ class get_locale {

public:
get_locale(bool localized, locale_ref loc) : has_locale_(localized) {
#ifndef FMT_STATIC_THOUSANDS_SEPARATOR
if (localized)
::new (&locale_) std::locale(loc.template get<std::locale>());
#endif
}
~get_locale() {
if (has_locale_) locale_.~locale();
Expand Down
6 changes: 5 additions & 1 deletion include/fmt/format.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1069,6 +1069,8 @@ template <typename Locale> class format_facet : public Locale::facet {
}
};

FMT_END_EXPORT

namespace detail {

// Returns true if value is negative, false otherwise.
Expand Down Expand Up @@ -3865,6 +3867,7 @@ FMT_API void report_error(format_func func, int error_code,
const char* message) noexcept;
} // namespace detail

FMT_BEGIN_EXPORT
FMT_API auto vsystem_error(int error_code, string_view format_str,
format_args args) -> std::system_error;

Expand Down Expand Up @@ -4285,6 +4288,8 @@ extern template FMT_API auto decimal_point_impl(locale_ref) -> char;
extern template FMT_API auto decimal_point_impl(locale_ref) -> wchar_t;
#endif // FMT_HEADER_ONLY

FMT_END_EXPORT
Copy link
Contributor

Choose a reason for hiding this comment

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

Why exclude native_formatter<T, Char, TYPE>::format from export? If you get an error please post it here for future reference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The error is:

In file included from fmt/src/fmt.cc:99:
In file included from fmt/include/fmt/args.h:17:
In file included from fmt/include/fmt/args.h:17:
fmt/include/fmt/format.h:4293:64: error: cannot export 'format' as it is not at namespace scope
 4293 | FMT_CONSTEXPR FMT_INLINE auto native_formatter<T, Char, TYPE>::format(
      |                               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^
fmt/include/fmt/format.h:4295:7: error: invalid use of non-static data member 'specs_'
fmt/include/fmt/format.h:4295:7: error: 'specs_' is a private member of 'native_formatter<T, Char, TYPE>'
fmt/include/fmt/base.h:2796:30: note: declared private here
 4295 |   if (specs_.width_ref.kind == arg_id_kind::none &&
 2796 |   dynamic_format_specs<Char> specs_;
      |       ^~~~~~
      |                              ^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe useful: llvm/llvm-project#61890

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So if you wanna export native_formatter<T, Char, TYPE>::format you should export native_formatter<T, Char, TYPE> instead. I tried and it compiled. If you think it okay I can commit it.


template <typename T, typename Char, type TYPE>
template <typename FormatContext>
FMT_CONSTEXPR FMT_INLINE auto native_formatter<T, Char, TYPE>::format(
Expand All @@ -4300,7 +4305,6 @@ FMT_CONSTEXPR FMT_INLINE auto native_formatter<T, Char, TYPE>::format(
return write<Char>(ctx.out(), val, specs, ctx.locale());
}

FMT_END_EXPORT
} // namespace detail

FMT_BEGIN_EXPORT
Expand Down
Loading