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

Basics of formatting at compile-time #1993

Closed

Conversation

alexezeder
Copy link
Contributor

I need to say that this PR does not introduce fully-functioning formatting via {fmt} at compile-time. By fully-functioning I mean that I handled only a few cases (one integer, string, string with integer) of the entire functionality of this library. In my opinion, it's enough to make some kind of demonstration, so everybody can review the basics of the provided implementation and discuss it with me. Or reject it, in this case, I will be finally free.

There are a few things that I should mention:

  • custom back_insert_iterator used because std:: one is not constexpr'ed in all compilers I know, but it should be in future
  • data.h header is necessary because data from struct data is needed at compile-time but cannot be used there, maybe a separate header is not the best solution...
  • temporary CI job, that uses only one compiler that can compile my creation, created to run a test of compile-time formatting

What functionality of the library is not available at compile-time with this PR:

  • pointers formatting - std::bit_cast is not available yet, so it's not possible yet
  • floating-point numbers formatting - looks like it's possible, but takes some time
  • format specifications (padding, precision, etc.) - haven't tried, takes some time
  • colors, locales, other functionality I'm not aware of - haven't tried, takes some time

@vitaut
Copy link
Contributor

vitaut commented Nov 10, 2020

Thanks for the PR. I will take a closer look later but a few quick comments: please don't split fmt/format.h into multiple headers and don't introduce new GitHub Actions workflows.

@alexezeder
Copy link
Contributor Author

please don't split fmt/format.h into multiple headers

Ok, I will move all these values back format.h then. I realized that it can be done this way too late 🙂

don't introduce new GitHub Actions workflows

It's temporary. Right now CI config is a bit... bad and likely be changed in the future. But I will revert this commit and this time also rebase, so there won't be any problem with that.

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.

Cool stuff!

My main comment is that this should go through the compile-time API (https://github.com/fmtlib/fmt/blob/master/include/fmt/compile.h) rather than the runtime one. Then you won't have to worry about making buffer management constexpr, only provide a constexpr-ready iterator.

I also left a few comments inline.

Comment on lines +255 to +268
#if defined(FMT_ENABLE_COMPILE_TIME_FORMATTING)
# if !defined(FMT_HEADER_ONLY) || __cplusplus < 202002L
# error "compile time formatting could not be enabled"
# endif
# define FMT_CTF_CONSTEXPR constexpr
# define FMT_CTF_CUSTOM_BACK_INSERT_ITERATOR 1
# define FMT_CTF_CHECK std::is_constant_evaluated()
# define FMT_CTF_ENABLED 1
#else
# define FMT_CTF_CONSTEXPR
# define FMT_CTF_CUSTOM_BACK_INSERT_ITERATOR 0
# define FMT_CTF_CHECK false
# define FMT_CTF_ENABLED 0
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't depend on FMT_ENABLE_COMPILE_TIME_FORMATTING. Instead use the normal {fmt} macros such as FMT_CONSTEXPR adding standard-specific variants if necessary, e.g.

#ifdef __cplusplus >= 201703L
#  define FMT_CONSTEXPR17 FMT_CONSTEXPR
#endif

Also I suggest moving constexprification to a separate PR since it's a pretty mechanical change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds like a lot of work, but I agree that I should be done this way. In the PR I simplify some things to get feedback for other things.

Comment on lines +665 to +670
#if FMT_CTF_CUSTOM_BACK_INSERT_ITERATOR
template <typename Container> class back_insert_iterator;
#else
template <typename Container>
using back_insert_iterator = std::back_insert_iterator<Container>;
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a new iterator if needed but please leave std::back_insert_iterator handling as before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what do you mean by std::back_insert_iterator handling, because this iterator is in the core of {fmt}. For me, this iterator is similar to string_view. That's why I propose to use a custom one all over the library, but only when an std:: one doesn't meet some requirements. Could you give me more details, please?

Copy link
Contributor

Choose a reason for hiding this comment

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

Consider the following example that uses format string compilation (https://godbolt.org/z/E9WYss):

#include <fmt/compile.h>

int main() {
  char buffer[10];
  fmt::format_to(buffer, FMT_COMPILE("{}"), 42);
  return buffer[0];
}

This does almost everything at compile time already only calling the write function at runtime:

main:
        sub     rsp, 24
        mov     esi, 42
        lea     rdi, [rsp+6]
        call    char* fmt::v7::detail::write<char, char*, int, 0>(char*, int)
        movsx   eax, BYTE PTR [rsp+6]
        add     rsp, 24
        ret

So you shouldn't change any buffers which are only relevant to the type-erased API. Instead, you should make write constexpr (which I guess you already do) and integrate with the format string compilation API (FMT_COMPILE). back_insert_iterator also becomes irrelevant since you can provide any iterator to the write functions.

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, got it, I definitely missed that. I will try to rework the current implementation using compile-time API then.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll close the issue for now then.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, it doesn't make much sense to change the type-erased functions in fmt/core.h or fmt/format.h for reasons discussed earlier. Only the format_to in fmt/compile.h needs to be changed or a new variant of it added if that proves too difficult.

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, looks like:

  • another function like format_to but with a different name
  • using FMT_COMPILE in format_to to force the one that uses compiled_string

are only options here. Both of these options mean that using the library at compile-time will be slightly different from using it at runtime. Both of these options force to not use format_to from core.h because otherwise, it's not clear to the compiler which version to choose.

Anyway, I will continue with FMT_COMPILE approach to present an updated implementation of compile-time formatting as soon as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vitaut Should I open a new PR or force-push to this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest opening a new one.

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 #2019

@alexezeder
Copy link
Contributor Author

My main comment is that this should go through the compile-time API (https://github.com/fmtlib/fmt/blob/master/include/fmt/compile.h) rather than the runtime one. Then you won't have to worry about making buffer management constexpr, only provide a constexpr-ready iterator.

Ok, I will try to use compile-time API at compile-time in runtime functions (did I get it right?), however, I'm not sure which part of buffer management will it ease for me.

@vitaut vitaut closed this Nov 12, 2020
@alexezeder alexezeder deleted the feature/compile_time_formatting branch January 15, 2021 21:14
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.

2 participants