Skip to content

Feature/add libfmt #182

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

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ jobs:
clang_format: true
clang_format_ignore: |
src/eckit/contrib
third-party
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we creating a second directory here. I don't see libfmt as a dependency being conceptually meaningfully different to xxHash. They are both third party contribs. Please put in the 'original' location.

Copy link
Contributor

Choose a reason for hiding this comment

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

well, it is a third-party library ... same for xxHash. The naming contribution is just not appropriate for these because they are no contributions from externals provided just for this purpose.

Also note that in #187 it is to discussed to move them to third_party directory as well, and also use git submodules properly...

However, we want to keep the discussion with the git submodules separate. That's why this library is still vendored.

Copy link
Member

Choose a reason for hiding this comment

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

Semantics... contrib is the directory for external contribution; personally I never heard of the term "vendored" before in this context either.
If instead this is agreed to go to a new directory third-party, then I propose to also move xxHash to this third-party to be consistent at least.

secrets: inherit

# Run CI of private downstream packages on self-hosted runners
Expand Down
3 changes: 3 additions & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -362,6 +362,9 @@ if( NOT ${CMAKE_USE_PTHREADS_INIT} )
message( FATAL_ERROR "Only pthreads supported - thread library found is [${THREADS_LIBRARIES}]" )
endif()

### vendored dependencies
add_subdirectory(third-party)

############################################################################################
# sources

Expand Down
4 changes: 4 additions & 0 deletions eckit-import.cmake.in
Original file line number Diff line number Diff line change
@@ -1 +1,5 @@
set( ECKIT_LIBRARIES @ECKIT_LIBRARIES@ )

include( CMakeFindDependencyMacro )

find_dependency(fmt REQUIRED HINTS ${CMAKE_CURRENT_LIST_DIR}/../eckit/third-party/fmt-11.1.4 @fmt_DIR@)
15 changes: 11 additions & 4 deletions src/eckit/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -840,15 +840,22 @@ list( APPEND eckit_testing_srcs
testing/Test.h
)

list( APPEND eckit_format_srcs
format/Format.h
format/Format.cc
)

list( APPEND eckit_dirs
bases
compat
config
container
exception
filesystem
format
io
log
maths
memory
message
net
Expand All @@ -857,14 +864,13 @@ list( APPEND eckit_dirs
persist
runtime
serialisation
thread
transaction
value
maths
system
testing
thread
transaction
types
utils
value
)

foreach( dir ${eckit_dirs} )
Expand Down Expand Up @@ -952,6 +958,7 @@ ecbuild_add_library(
${THREADS_LIBRARIES}
${CMAKE_DL_LIBS}
${ECKIT_SYSTEM_EXTRA_LIBS}
fmt::fmt

LINKER_LANGUAGE CXX )

Expand Down
9 changes: 9 additions & 0 deletions src/eckit/format/Format.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
#include "Format.h"

namespace eckit {

std::ostream_iterator<const char&> makeOrForwardOutputiterator(std::ostream& os) {
return std::ostream_iterator<const char&>(os);
}

} // namespace eckit
109 changes: 109 additions & 0 deletions src/eckit/format/Format.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
/*
* (C) Copyright 2025- ECMWF.
*
* This software is licensed under the terms of the Apache Licence Version 2.0
* which can be obtained at http://www.apache.org/licenses/LICENSE-2.0.
* In applying this licence, ECMWF does not waive the privileges and immunities
* granted to it by virtue of its status as an intergovernmental organisation nor
* does it submit to any jurisdiction.
*/
#pragma once

#include <fmt/chrono.h>
#include <fmt/compile.h>
#include <fmt/format.h>
#include <fmt/ranges.h>
#include <fmt/std.h>

#include <iterator>
#include <type_traits>

/*
* Format wrappers around libfmt
*
* For most cases it is encouraged to use the macro `eckit_format`. It will perform compile time checks.
*
* For a very specific timecritical cases `eckit_format_cc` can be used to produce very optimized formatting code.
* Disadvantag: more binary code
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Disadvantag: more binary code
* Disadvantage: more binary code

*
* For other cases where the format string may dynamicall be configured somethere else, the functions `eckit::format`
* and `eckit::format_to` can be used - here checks are performed at runtime and may throw.
*/


#define ENABLE_FORMAT(typ) \
template <> \
struct fmt::formatter<typ> : fmt::ostream_formatter {}
Comment on lines +34 to +36
Copy link
Member

Choose a reason for hiding this comment

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

This macro should not be so generic but be prefixed with ECKIT_


/// Format a string with compile time checks.
/// @param formatString to use, see: <https://fmt.dev/11.1/syntax/> for description of syntax.
/// Must be known at compiletime
/// @param ... args to be upplied into formatString
#define eckit_format(str, ...) fmt::format(FMT_STRING(str), ##__VA_ARGS__)

/// Format s string with compile time optimizations.
/// Converts formatString into a format string that will be parsed at compile time and converted into efficient
/// formatting code.
/// @note Format string compilation can generate more binary code compared to the default API and is only recommended in
/// places where formatting is a performance bottleneck.
/// @param formatString to use, see: <https://fmt.dev/11.1/syntax/> for description of syntax.
/// @param ... args to be upplied into formatString
/// @throws fm::format_error if args cannot be applied to formatString or formatString syntax is invalid.
#define eckit_format_cc(str, ...) fmt::format(FMT_COMPILE(str), ##__VA_ARGS__)


/// Format a string with compile time checks and output to an outputiterator or ostream.
/// @param out OutputIterator or ostream
/// @param formatString to use, see: <https://fmt.dev/11.1/syntax/> for description of syntax.
/// Must be known at compiletime
/// @param ... args to be upplied into formatString
#define eckit_format_to(out, str, ...) \
fmt::format_to(eckit::makeOrForwardOutputiterator(out), FMT_STRING(str), ##__VA_ARGS__)

/// Format s string with compile time optimizations and output to an outputiterator or ostream.
/// Converts formatString into a format string that will be parsed at compile time and converted into efficient
/// formatting code.
/// @note Format string compilation can generate more binary code compared to the default API and is only recommended in
/// places where formatting is a performance bottleneck.
/// @param out OutputIterator or ostream
/// @param formatString to use, see: <https://fmt.dev/11.1/syntax/> for description of syntax.
/// @param ... args to be upplied into formatString
/// @throws fm::format_error if args cannot be applied to formatString or formatString syntax is invalid.
#define eckit_format_to_cc(out, str, ...) \
fmt::format_to(eckit::makeOrForwardOutputiterator(out), FMT_COMPILE(str), ##__VA_ARGS__)

namespace eckit {

template <typename OutputIt, std::enable_if_t<!std::is_base_of_v<std::ostream, std::decay_t<OutputIt>>, bool> = true>
OutputIt&& makeOrForwardOutputiterator(OutputIt&& outputIt) {
return std::forward<OutputIt>(outputIt);
}

std::ostream_iterator<const char&> makeOrForwardOutputiterator(std::ostream& os);


/// Format a string with compile time checks.
/// @param formatString to use, see: <https://fmt.dev/11.1/syntax/> for description of syntax.
/// Must be known at compiletime
/// @param ... args to be upplied into formatString
/// @throws fmt::format_error
template <typename StringType, typename... Args>
std::string str_format(StringType&& formatString, Args&&... args) {
return fmt::format(fmt::runtime(std::forward<StringType>(formatString)), std::forward<Args>(args)...);
}


/// Format a string with compile time checks to an output iterator
/// @param outputIt output iterator or ostream to write to
/// @param formatString to use, see: <https://fmt.dev/11.1/syntax/> for description of syntax.
/// Must be known at compiletime
/// @param ... args to be upplied into formatString
/// @throws fmt::format_error
template <typename OutputIt, typename StringType, typename... Args>
void str_format_to(OutputIt&& outputIt, StringType&& formatString, Args&&... args) {
fmt::format_to(makeOrForwardOutputiterator(std::forward<OutputIt>(outputIt)),
fmt::runtime(std::forward<StringType>(formatString)), std::forward<Args>(args)...);
}


} // namespace eckit
22 changes: 22 additions & 0 deletions src/eckit/log/Log.h
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#define eckit_log_Log_h

#include "eckit/deprecated.h"
#include "eckit/format/Format.h"
#include "eckit/log/Channel.h"
#include "eckit/log/UserChannel.h"

Expand Down Expand Up @@ -162,6 +163,27 @@ class Voidify {
#define LOG_DEBUG_LIB(lib) \
static_cast<void>(0), !(lib::instance().debug()) ? (void)0 : eckit::Voidify() & eckit::Log::debug<lib>()

/// Format message to debug channel of a library
#define DEBUG_MSG(lib, str, ...) \
if (lib::instance().debug()) { \
fmt::format_to(eckit::makeOrForwardOutputiterator(eckit::Log::debug<lib>()), FMT_STRING(str), ##__VA_ARGS__); \
}

/**
* Not to be used directly. Use (WARNING|INFO|ERROR|PANIC)MSG instead.
*/
#define __XXX_MSG(type, str, ...) \
fmt::format_to(eckit::makeOrForwardOutputiterator(Log::##type()), FMT_STRING(str), ##__VA_ARGS__);

#define WARNING_MSG(str, ...) __XXX_MSG(warning, str, __VA_ARGS__)

#define INFO_MSG(str, ...) __XXX_MSG(info, str, __VA_ARGS__)

#define ERROR_MSG(str, ...) __XXX_MSG(error, str, __VA_ARGS__)

#define PANIC_MSG(str, ...) __XXX_MSG(panic, str, __VA_ARGS__)


//----------------------------------------------------------------------------------------------------------------------

} // namespace eckit
Expand Down
24 changes: 6 additions & 18 deletions src/eckit/types/FloatCompare.cc
Original file line number Diff line number Diff line change
@@ -1,35 +1,23 @@
// #include <cmath>

// Some of the math.h/cmath functions are not clean when switching to C++11
#if __cplusplus <= 199711L
#include <cmath>
#else
#include <cmath>
#define fpclassify(x) std::fpclassify((x))
#define isinf(x) std::isinf((x))
#define isnan(x) std::isnan((x))
#define signbit(x) std::signbit((x))
#endif

#include <sys/types.h>
#include <cmath>
#include <limits>

#include "eckit/exception/Exceptions.h"
#include "eckit/types/FloatCompare.h"

using std::fpclassify;
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems unrelated.

We weren't using the std:: ones as we had issues on some compiler combinations. Not sure I want to couple this change in this context?

Copy link
Contributor

Choose a reason for hiding this comment

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

we included the eckit/format/Format.h in eckit/log/Log.h - this was then included in FloatCompare

Here some macros to math functions were used to avoid having to prefix std:: ... however as they were declared as macros, they caused a lot of conflicts with the included libtfmt headers.

In that sense using the using std::function; approach is much cleaner. I'm not sure if this approach was properly tested before? Moreover we are now using C++17 - I'd be really supprised if there are still compilers complaining about these things. Also the CI passes.

using std::isinf;
using std::isnan;
using std::signbit;

namespace eckit::types {

//----------------------------------------------------------------------------------------------------------------------

namespace detail {

// FIXME: The following functions are available in std:: as of C++11:
// * fpclassify
// * isinf
// * isnan
// * signbit
// For the moment we have to use the (non namespaced) versions from math.h

template <class T>
inline T abs(T);

Expand Down
1 change: 1 addition & 0 deletions tests/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ add_subdirectory( config )
add_subdirectory( container )
add_subdirectory( exception )
add_subdirectory( filesystem )
add_subdirectory( format )
add_subdirectory( geometry )
add_subdirectory( io )
add_subdirectory( large_file )
Expand Down
12 changes: 12 additions & 0 deletions tests/format/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
ecbuild_add_test( TARGET eckit_test_format
SOURCES test_format.cc
LIBS eckit )

ecbuild_add_test( TARGET eckit_test_format_to
SOURCES test_format_to.cc
LIBS eckit )

ecbuild_add_test( TARGET eckit_test_format_log
SOURCES test_format_log.cc
LIBS eckit )

60 changes: 60 additions & 0 deletions tests/format/test_format.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
/*
* (C) Copyright 2025- ECMWF.
*
* This software is licensed under the terms of the Apache Licence Version 2.0
* which can be obtained at http://www.apache.org/licenses/LICENSE-2.0.
* In applying this licence, ECMWF does not waive the privileges and immunities
* granted to it by virtue of its status as an intergovernmental organisation nor
* does it submit to any jurisdiction.
*/
#include <eckit/testing/Test.h>

#include "eckit/format/Format.h"

CASE("Can use eckit_format macro") {
EXPECT_EQUAL(eckit_format("Hello {} {}", 1, 2), std::string("Hello 1 2"));
EXPECT_EQUAL(eckit_format(std::string_view("Hello {} {}"), 1, 2), std::string("Hello 1 2"));
}

CASE("Can use eckit::str_format") {
EXPECT_EQUAL(eckit::str_format("Hello {} {}", 1, 2), std::string("Hello 1 2"));
EXPECT_EQUAL(eckit::str_format(std::string_view("Hello {} {}"), 1, 2), std::string("Hello 1 2"));
EXPECT_EQUAL(eckit::str_format(std::string("Hello {} {}"), 1, 2), std::string("Hello 1 2"));
std::string str("Hello {} {}");
std::string_view str_v(str);
EXPECT_EQUAL(eckit::str_format(str_v, 1, 2), std::string("Hello 1 2"));
}

CASE("eckit::str_format throws on format erors") {
EXPECT_THROWS_AS(eckit::str_format("Hello {} {} {}", 1, 2), fmt::format_error);
EXPECT_THROWS_AS(eckit::str_format(std::string("Hello {} {} {}"), 1, 2), fmt::format_error);
EXPECT_THROWS_AS(eckit::str_format(std::string_view("Hello {} {} {}"), 1, 2), fmt::format_error);
EXPECT_THROWS_AS(eckit::str_format("Hello {} {} {", 1, 2), fmt::format_error);
EXPECT_THROWS_AS(eckit::str_format(std::string("Hello {} {} {"), 1, 2), fmt::format_error);
EXPECT_THROWS_AS(eckit::str_format(std::string_view("Hello {} {} {"), 1, 2), fmt::format_error);
EXPECT_THROWS_AS(eckit::str_format("Hello {:p} {} {}", 1, 2), fmt::format_error);
EXPECT_THROWS_AS(eckit::str_format(std::string("Hello {:p} {} {}"), 1, 2), fmt::format_error);
EXPECT_THROWS_AS(eckit::str_format(std::string_view("Hello {:p} {} {}"), 1, 2), fmt::format_error);
}

struct MyType {
int x{5};
};
std::ostream& operator<<(std::ostream& out, const MyType& mt) {
return out << "MyType{" << mt.x << "}";
}
ENABLE_FORMAT(MyType);

CASE("ENABLE_FORMAT works") {
EXPECT_EQUAL(eckit::str_format("Hello {}", MyType{}), std::string("Hello MyType{5}"));
}

ENABLE_FORMAT(eckit::CodeLocation);
CASE("ENABLE_FORMAT CodeLocation") {
// This is just an example
std::cout << eckit::str_format("{}", Here());
}

int main(int argc, char** argv) {
return eckit::testing::run_tests(argc, argv);
}
Loading
Loading