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.

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 UDL as replacement for FMT_COMPILE #2043

Merged
merged 6 commits into from
Dec 7, 2020

Conversation

alexezeder
Copy link
Contributor

@alexezeder alexezeder commented Dec 1, 2020

Right now {fmt} has a special macro to invoke compile-time API functions - FMT_COMPILE. This macro can be replaced with UDL for modern compilers that support C++20. The chosen name in this PR for this feature is _cf.

fmt::format(FMT_COMPILE("{}"), 42); // 🙁 not modern
fmt::format("{}"_cf, 42); // 🙂 modern as hell

Note that only GCC 9.3+ support that. Clang (trunk) is also able to compile this code, but it doesn't have __cpp_nontype_template_parameter_class defined, I hope it'll be fixed in Clang 12 release.
Proof on Compiler Explorer

constexpr is used instead of consteval (even though it would make more sense) to lower GCC version.

Also, I should note that this PR is based on my own ideas and the following comments: #1874 (comment), #613 (comment).

Copy link
Contributor

@vitaut vitaut left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Mostly looks good, just a few nits inline.

copy_str<Char, const Char*, Char*>(static_cast<const Char*>(str), str + N,
data);
}
Char data[N]{};
Copy link
Contributor

Choose a reason for hiding this comment

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

{} seems unnecessary, please remove.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's either {} here or : data{} initialization in constructor for GCC 9.3, otherwise:

<source>:8:13: error: member 'fmt::v7::detail::fixed_string<char, 2>::data' must be initialized by mem-initializer in 'constexpr' constructor
<source>:12:8: note: declared here
   12 |   Char data[N];
      |        ^~~~

include/fmt/compile.h Outdated Show resolved Hide resolved
include/fmt/compile.h Outdated Show resolved Hide resolved
@alexezeder alexezeder force-pushed the feature/compile_literal branch from df32877 to 855e488 Compare December 2, 2020 18:17
@alexezeder alexezeder requested a review from vitaut December 2, 2020 18:42
@yeswalrus
Copy link
Contributor

I thought the plan was to just make compile-time checks the default in c++20?

@alexezeder
Copy link
Contributor Author

alexezeder commented Dec 2, 2020

@yeswalrus I think you are confusing FMT_STRING and FMT_COMPILE. They are similar, they even use the same FMT_STRING_IMPL macro under the hood. But FMT_STRING is used to indicate that format string should be just checked, while FMT_COMPILE is used to indicate that format string should be baked in form of some compiled format object that has prepared parts for formatting.
This PR is about the FMT_COMPILE which invokes compile-time API.

@yeswalrus
Copy link
Contributor

Correct, my mistake.

Comment on lines 16 to 20
#if !defined(FMT_USE_NONTYPE_TEMPLATE_PARAMETERS) && \
defined(__cpp_nontype_template_parameter_class) && \
(!FMT_GCC_VERSION || FMT_GCC_VERSION >= 903)
# define FMT_USE_NONTYPE_TEMPLATE_PARAMETERS
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

Please make FMT_USE_NONTYPE_TEMPLATE_PARAMETERS defined to 1/0 for consistency with other similar macros and to make it possible to disable the feature. See e.g.

fmt/include/fmt/core.h

Lines 178 to 185 in 33f9a6d

#ifndef FMT_USE_INLINE_NAMESPACES
# if FMT_HAS_FEATURE(cxx_inline_namespaces) || FMT_GCC_VERSION >= 404 || \
(FMT_MSC_VER >= 1900 && !_MANAGED)
# define FMT_USE_INLINE_NAMESPACES 1
# else
# define FMT_USE_INLINE_NAMESPACES 0
# endif
#endif

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

@@ -37,6 +43,27 @@ struct is_compiled_string : std::is_base_of<compiled_string, S> {};
*/
#define FMT_COMPILE(s) FMT_STRING_IMPL(s, fmt::detail::compiled_string)

#ifdef FMT_USE_NONTYPE_TEMPLATE_PARAMETERS
template <typename Char, size_t N> struct fixed_string {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is fixed_string actually needed? Can we directly use an array instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like yes, it's actually needed. The raw C-style array should be wrapped in some type, and this wrapper-type should also have a constructor with const Char (&str)[N] as an argument. Of course, I can be wrong here, but all my attempts ended up with nothing working.

But I realized that the deduction guide for fixed_string can be removed now because it holds a null-terminated string.

@alexezeder alexezeder requested a review from vitaut December 7, 2020 20:46
@vitaut vitaut merged commit 5de0bc1 into fmtlib:master Dec 7, 2020
@vitaut
Copy link
Contributor

vitaut commented Dec 7, 2020

Merged, thanks.

@alexezeder alexezeder deleted the feature/compile_literal branch January 15, 2021 21:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants