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

Add support of most format_specs for formatting at compile-time #2056

Merged
merged 14 commits into from
Dec 25, 2020
Merged

Add support of most format_specs for formatting at compile-time #2056

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
725eedf
add tests for specs, std::string_view -> fmt::basic_string_view in te…
alexezeder Dec 8, 2020
aead58e
add constexpr where it's needed
alexezeder Dec 9, 2020
62abf10
add missing `0` spec test
alexezeder Dec 9, 2020
6742642
quick fix for MSVC 19.0.24241.7
alexezeder Dec 9, 2020
de42f13
use `is_constant_evaluated()`` to initialize class `value` with string
alexezeder Dec 15, 2020
fec7c1a
use `FMT_NOEXCEPT` instead of `FMT_DETECTED_NOEXCEPT`
alexezeder Dec 22, 2020
2980dd3
remove braces for `if` in `value` constuctor
alexezeder Dec 22, 2020
b8d973a
mark `formatter<>::format()` functions as `const`
alexezeder Dec 22, 2020
6714f90
enable formatting at compile-time for GCC 10.2+
alexezeder Dec 22, 2020
8131d4f
fix macro check for formatting at compile-time on GCC 10.2+
alexezeder Dec 24, 2020
14ce33e
make `hex_digits` and shifts in `basic_data` constexpr
alexezeder Dec 25, 2020
4762f64
remove non-const `format` method from `formatter`
alexezeder Dec 25, 2020
5cca27f
add missing `constexpr` in definitions of static members
alexezeder Dec 25, 2020
1b1c3ce
replace `FMT_CONSTEXPR20` with `FMT_CONSTEXPR`
alexezeder Dec 25, 2020
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
18 changes: 9 additions & 9 deletions include/fmt/compile.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -466,10 +466,10 @@ struct is_compiled_format<field<Char, T, N>> : std::true_type {};
// A replacement field that refers to argument N and has format specifiers.
template <typename Char, typename T, int N> struct spec_field {
using char_type = Char;
mutable formatter<T, Char> fmt;
formatter<T, Char> fmt;

template <typename OutputIt, typename... Args>
OutputIt format(OutputIt out, const Args&... args) const {
constexpr OutputIt format(OutputIt out, const Args&... args) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What exactly is the problem with const?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All these problems started with mutable, GCC thinks that mutable formatter<T, Char> fmt in spec_field structure cannot be used at compile-time. As I can see Clang <7.0 also thinks the same way, this is why the side effect of provided PR is that Clang <7.0 with C++17 support can also use FMT_COMPILE with specs.

I failed to get a small code example which raises this error, I tried to create a failing example by recreating all significant parts of the error stack trace, but strangely it works. But I can present you the huge example with this error, this example is created by combining all needed headers of {fmt} together (version from this PR, except compile.h) and by making everything needed as constexpr in compile.h without mutable-related changes. Here it is on Compiler Explorer: https://godbolt.org/z/n1Kz38

Since the behaviour of GCC 10.2 and Clang <7.0 is kind of similar, I also tried to recreate the error with Clang 6.0 with my small code example that IMHO should fail - no luck, so looks like I'm missing something here. But of course, it fails to compile with a bit modified huge example where all {fmt} headers are combined: https://godbolt.org/z/P17TGG

So, this became non-const to eliminate mutable, const CompiledFormat& cf became non-const references because method format is no more const, constexpr auto compiled became just auto compiled because const object cannot be passed via non-const reference. This is my explanation for why I made these changes, of course, I can miss some more elegant solution, especially considering that I failed to recreate this error in a small example. In case it's true and more elegant solution exist I'll be glad to implement it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the correct solution is to make formatter::format const and keep this format function const.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, by in my opinion, there is a potential problem with this solution, with making format() function in formatters const - maybe if a some of formatters have format() function non-const on purpose then it cannot be used at compile-time. I just don't know:

  1. why all or almost all formatters currently defined in {fmt} has non-const format() function?
  2. how rare it is that format() function in custom formatters is non-const on purpose?

I mean, I can definitely try to make this function const for formatters which are used in test, but since you have better perspective here it will be helpful if you agree or disagree with me for this potential problem.

Actually, I don't even consider to go this way because some already created formatters would be unavailable at compile-time. On the other hand the code I have in this PR right now does not change formatters at all.

Copy link
Contributor

@vitaut vitaut Dec 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

why all or almost all formatters currently defined in {fmt} has non-const format() function?

No good reason, just less typing =).

how rare it is that format() function in custom formatters is non-const on purpose?

I don't think it should necessary at all. format shouldn't mutate formatter, only apply format specifiers stored in it to the input (or rather output =)).

already created formatters would be unavailable at compile-time

We can fix this for all {fmt} formatters and later provide a workaround for user-defined ones.

Copy link
Contributor Author

@alexezeder alexezeder Dec 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Ok, I reverted back most of the changes in compile.h and mark all of formatters as const, at least those I found.

and later provide a workaround for user-defined ones.

But there is no such thing as workarounds at compile-time 🙂, that why it's so cool to check your code just by making it execute at compile-time. Of course, I'm talking about workarounds that are for removing the constness of objects, not interface workarounds.

Copy link
Contributor

Choose a reason for hiding this comment

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

The workaround is to copy the formatter when needed.

// This ensures that the argument type is convertile to `const T&`.
const T& arg = get<N>(args...);
const auto& vargs =
Expand All @@ -488,7 +488,7 @@ template <typename L, typename R> struct concat {
using char_type = typename L::char_type;

template <typename OutputIt, typename... Args>
constexpr OutputIt format(OutputIt out, const Args&... args) const {
constexpr OutputIt format(OutputIt out, const Args&... args) {
out = lhs.format(out, args...);
return rhs.format(out, args...);
}
Expand Down Expand Up @@ -635,7 +635,7 @@ FMT_DEPRECATED auto compile(const Args&... args)
template <typename CompiledFormat, typename... Args,
typename Char = typename CompiledFormat::char_type,
FMT_ENABLE_IF(detail::is_compiled_format<CompiledFormat>::value)>
FMT_INLINE std::basic_string<Char> format(const CompiledFormat& cf,
FMT_INLINE std::basic_string<Char> format(CompiledFormat& cf,
const Args&... args) {
basic_memory_buffer<Char> buffer;
cf.format(detail::buffer_appender<Char>(buffer), args...);
Expand All @@ -644,7 +644,7 @@ FMT_INLINE std::basic_string<Char> format(const CompiledFormat& cf,

template <typename OutputIt, typename CompiledFormat, typename... Args,
FMT_ENABLE_IF(detail::is_compiled_format<CompiledFormat>::value)>
constexpr OutputIt format_to(OutputIt out, const CompiledFormat& cf,
constexpr OutputIt format_to(OutputIt out, CompiledFormat& cf,
const Args&... args) {
return cf.format(out, args...);
}
Expand Down Expand Up @@ -674,14 +674,14 @@ FMT_INLINE std::basic_string<typename S::char_type> format(const S&,
return fmt::to_string(detail::first(args...));
}
#endif
constexpr auto compiled = detail::compile<Args...>(S());
auto compiled = detail::compile<Args...>(S());
alexezeder marked this conversation as resolved.
Show resolved Hide resolved
return format(compiled, std::forward<Args>(args)...);
}

template <typename OutputIt, typename CompiledFormat, typename... Args,
FMT_ENABLE_IF(std::is_base_of<detail::basic_compiled_format,
CompiledFormat>::value)>
constexpr OutputIt format_to(OutputIt out, const CompiledFormat& cf,
constexpr OutputIt format_to(OutputIt out, CompiledFormat& cf,
const Args&... args) {
using char_type = typename CompiledFormat::char_type;
using context = format_context_t<OutputIt, char_type>;
Expand All @@ -692,7 +692,7 @@ constexpr OutputIt format_to(OutputIt out, const CompiledFormat& cf,
template <typename OutputIt, typename S, typename... Args,
FMT_ENABLE_IF(detail::is_compiled_string<S>::value)>
FMT_CONSTEXPR OutputIt format_to(OutputIt out, const S&, const Args&... args) {
constexpr auto compiled = detail::compile<Args...>(S());
auto compiled = detail::compile<Args...>(S());
return format_to(out, compiled, args...);
}

Expand All @@ -714,7 +714,7 @@ template <typename OutputIt, typename S, typename... Args,
FMT_ENABLE_IF(detail::is_compiled_string<S>::value)>
format_to_n_result<OutputIt> format_to_n(OutputIt out, size_t n, const S&,
const Args&... args) {
constexpr auto compiled = detail::compile<Args...>(S());
auto compiled = detail::compile<Args...>(S());
auto it = format_to(detail::truncating_iterator<OutputIt>(out, n), compiled,
args...);
return {it.base(), it.count()};
Expand Down
72 changes: 38 additions & 34 deletions include/fmt/core.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -481,7 +481,7 @@ inline basic_string_view<Char> to_string_view(
}

template <typename Char>
inline basic_string_view<Char> to_string_view(basic_string_view<Char> s) {
constexpr basic_string_view<Char> to_string_view(basic_string_view<Char> s) {
return s;
}

Expand Down Expand Up @@ -938,9 +938,9 @@ struct arg_data<T, Char, NUM_ARGS, 0> {
T args_[NUM_ARGS != 0 ? NUM_ARGS : +1];

template <typename... U>
FMT_INLINE arg_data(const U&... init) : args_{init...} {}
FMT_INLINE const T* args() const { return args_; }
FMT_INLINE std::nullptr_t named_args() { return nullptr; }
FMT_CONSTEXPR arg_data(const U&... init) : args_{init...} {}
FMT_CONSTEXPR const T* args() const { return args_; }
FMT_CONSTEXPR std::nullptr_t named_args() { return nullptr; }
alexezeder marked this conversation as resolved.
Show resolved Hide resolved
};

template <typename Char>
Expand All @@ -961,7 +961,7 @@ void init_named_args(named_arg_info<Char>* named_args, int arg_count,
}

template <typename... Args>
FMT_INLINE void init_named_args(std::nullptr_t, int, int, const Args&...) {}
FMT_CONSTEXPR void init_named_args(std::nullptr_t, int, int, const Args&...) {}

template <typename T> struct is_named_arg : std::false_type {};

Expand Down Expand Up @@ -1073,17 +1073,17 @@ template <typename Context> class value {

constexpr FMT_INLINE value(int val = 0) : int_value(val) {}
constexpr FMT_INLINE value(unsigned val) : uint_value(val) {}
FMT_INLINE value(long long val) : long_long_value(val) {}
FMT_INLINE value(unsigned long long val) : ulong_long_value(val) {}
constexpr FMT_INLINE value(long long val) : long_long_value(val) {}
constexpr FMT_INLINE value(unsigned long long val) : ulong_long_value(val) {}
FMT_INLINE value(int128_t val) : int128_value(val) {}
FMT_INLINE value(uint128_t val) : uint128_value(val) {}
FMT_INLINE value(float val) : float_value(val) {}
FMT_INLINE value(double val) : double_value(val) {}
FMT_INLINE value(long double val) : long_double_value(val) {}
FMT_INLINE value(bool val) : bool_value(val) {}
FMT_INLINE value(char_type val) : char_value(val) {}
FMT_INLINE value(const char_type* val) { string.data = val; }
FMT_INLINE value(basic_string_view<char_type> val) {
constexpr FMT_INLINE value(bool val) : bool_value(val) {}
constexpr FMT_INLINE value(char_type val) : char_value(val) {}
FMT_CONSTEXPR value(const char_type* val) : string() { string.data = val; }
alexezeder marked this conversation as resolved.
Show resolved Hide resolved
FMT_CONSTEXPR value(basic_string_view<char_type> val) {
string.data = val.data();
string.size = val.size();
}
Expand Down Expand Up @@ -1406,7 +1406,7 @@ class locale_ref {
const void* locale_; // A type-erased pointer to std::locale.

public:
locale_ref() : locale_(nullptr) {}
constexpr locale_ref() : locale_(nullptr) {}
template <typename Locale> explicit locale_ref(const Locale& loc);

explicit operator bool() const FMT_NOEXCEPT { return locale_ != nullptr; }
Expand Down Expand Up @@ -1437,7 +1437,7 @@ template <typename T> int check(unformattable) {
"formatter<T> specialization: https://fmt.dev/latest/api.html#udt");
return 0;
}
template <typename T, typename U> inline const U& check(const U& val) {
template <typename T, typename U> constexpr const U& check(const U& val) {
return val;
}

Expand All @@ -1446,7 +1446,7 @@ template <typename T, typename U> inline const U& check(const U& val) {
// another (not recommended).
template <bool IS_PACKED, typename Context, type, typename T,
FMT_ENABLE_IF(IS_PACKED)>
inline value<Context> make_arg(const T& val) {
constexpr value<Context> make_arg(const T& val) {
return check<T>(arg_mapper<Context>().map(val));
}

Expand Down Expand Up @@ -1480,28 +1480,30 @@ template <typename OutputIt, typename Char> class basic_format_context {
Constructs a ``basic_format_context`` object. References to the arguments are
stored in the object so make sure they have appropriate lifetimes.
*/
basic_format_context(OutputIt out,
basic_format_args<basic_format_context> ctx_args,
detail::locale_ref loc = detail::locale_ref())
constexpr basic_format_context(
OutputIt out, basic_format_args<basic_format_context> ctx_args,
detail::locale_ref loc = detail::locale_ref())
: out_(out), args_(ctx_args), loc_(loc) {}

format_arg arg(int id) const { return args_.get(id); }
format_arg arg(basic_string_view<char_type> name) { return args_.get(name); }
constexpr format_arg arg(int id) const { return args_.get(id); }
FMT_CONSTEXPR format_arg arg(basic_string_view<char_type> name) {
alexezeder marked this conversation as resolved.
Show resolved Hide resolved
return args_.get(name);
}
int arg_id(basic_string_view<char_type> name) { return args_.get_id(name); }
const basic_format_args<basic_format_context>& args() const { return args_; }

detail::error_handler error_handler() { return {}; }
FMT_CONSTEXPR detail::error_handler error_handler() { return {}; }
void on_error(const char* message) { error_handler().on_error(message); }

// Returns an iterator to the beginning of the output range.
iterator out() { return out_; }
FMT_CONSTEXPR iterator out() { return out_; }

// Advances the begin iterator to ``it``.
void advance_to(iterator it) {
if (!detail::is_back_insert_iterator<iterator>()) out_ = it;
}

detail::locale_ref locale() { return loc_; }
FMT_CONSTEXPR detail::locale_ref locale() { return loc_; }
};

template <typename Char>
Expand Down Expand Up @@ -1550,7 +1552,7 @@ class format_arg_store
: 0);

public:
format_arg_store(const Args&... args)
FMT_CONSTEXPR format_arg_store(const Args&... args)
:
#if FMT_GCC_VERSION && FMT_GCC_VERSION < 409
basic_format_args<Context>(*this),
Expand All @@ -1571,7 +1573,7 @@ class format_arg_store
\endrst
*/
template <typename Context = format_context, typename... Args>
inline format_arg_store<Context, Args...> make_format_args(
constexpr format_arg_store<Context, Args...> make_format_args(
const Args&... args) {
return {args...};
}
Expand Down Expand Up @@ -1644,33 +1646,35 @@ template <typename Context> class basic_format_args {
const format_arg* args_;
};

bool is_packed() const { return (desc_ & detail::is_unpacked_bit) == 0; }
constexpr bool is_packed() const {
return (desc_ & detail::is_unpacked_bit) == 0;
}
bool has_named_args() const {
return (desc_ & detail::has_named_args_bit) != 0;
}

detail::type type(int index) const {
FMT_CONSTEXPR detail::type type(int index) const {
int shift = index * detail::packed_arg_bits;
unsigned int mask = (1 << detail::packed_arg_bits) - 1;
return static_cast<detail::type>((desc_ >> shift) & mask);
}

basic_format_args(unsigned long long desc,
const detail::value<Context>* values)
constexpr basic_format_args(unsigned long long desc,
const detail::value<Context>* values)
: desc_(desc), values_(values) {}
basic_format_args(unsigned long long desc, const format_arg* args)
constexpr basic_format_args(unsigned long long desc, const format_arg* args)
: desc_(desc), args_(args) {}

public:
basic_format_args() : desc_(0) {}
constexpr basic_format_args() : desc_(0), args_(nullptr) {}

/**
\rst
Constructs a `basic_format_args` object from `~fmt::format_arg_store`.
\endrst
*/
template <typename... Args>
FMT_INLINE basic_format_args(const format_arg_store<Context, Args...>& store)
constexpr basic_format_args(const format_arg_store<Context, Args...>& store)
Comment on lines -1673 to +1688
Copy link
Contributor

Choose a reason for hiding this comment

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

Here and elsewhere: please keep FMT_INLINE.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I missed that FMT_INLINE has an attribute inside, and this line can use FMT_INLINE and constexpr. But those lines where FMT_INLINE was replaced by FMT_CONSTEXPR(20) will have double inline specifier, which makes some compilers unhappy and some very unhappy.

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 looks like one of these macros shouldn't set inline and that would be really strange if this macro is going to be FMT_INLINE.

I can give an example of Hedley, it does not fallback to inline in the case where constexpr is not available - https://github.com/nemequ/hedley/blob/5e50f6b735aeb4e09e3b2ad3d1717db3c0613bfa/hedley.h#L1269-L1276
and I think it's right because I can always mark a function as inline with or without constexpr, no matter that constexpr implies inline.

In case you agree with me here I can create a separate PR to remove inline from FMT_CONSTEXPR(20) macro. Otherwise I just don't know how to fix these problems without introducing FMT_CONSTEXPR(20)_NO_INLINE macro, which probably would be also bad naming because there is FMT_NOINLINE macro that expands to additional attribute...

Copy link
Contributor

Choose a reason for hiding this comment

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

In case you agree with me here I can create a separate PR to remove inline from FMT_CONSTEXPR(20) macro.

Sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done #2075

: basic_format_args(store.desc, store.data_.args()) {}

/**
Expand All @@ -1679,20 +1683,20 @@ template <typename Context> class basic_format_args {
`~fmt::dynamic_format_arg_store`.
\endrst
*/
FMT_INLINE basic_format_args(const dynamic_format_arg_store<Context>& store)
constexpr basic_format_args(const dynamic_format_arg_store<Context>& store)
: basic_format_args(store.get_types(), store.data()) {}

/**
\rst
Constructs a `basic_format_args` object from a dynamic set of arguments.
\endrst
*/
basic_format_args(const format_arg* args, int count)
constexpr basic_format_args(const format_arg* args, int count)
: basic_format_args(detail::is_unpacked_bit | detail::to_unsigned(count),
args) {}

/** Returns the argument with the specified id. */
format_arg get(int id) const {
FMT_CONSTEXPR format_arg get(int id) const {
format_arg arg;
if (!is_packed()) {
if (id < max_size()) arg = args_[id];
Expand Down
Loading