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 full support of format string parsing in compile-time API #2111

Conversation

alexezeder
Copy link
Contributor

@alexezeder alexezeder commented Jan 22, 2021

Some time ago, I wrote that I would try to add support of named arguments in compile-time API here. This took some time... especially when I realize that I can replace most of the functions written by me with functions that already exist in {fmt}.

Some points for the changes:

  • works with C++17 as it did before, I tried to make additional implementation for C++20 with non-type template parameters to eliminate unnecessary template instantiations, but I ended up recreating every used type to make them structural and facing with this fancy problem I was not even thinking about, so there is no additional C++20 implementation for now (and maybe forever)
  • custom (for compile-time API's FMT_COMPILE and _cf) parsing functions (compile_format_string() + parse_tail()) replaced with the usage of format_string_compiler, this gives full support of format string parsing, it's already written and tested code, and that's nice
  • one-step algorithm with recursion of compile_format_string() + parse_tail() functions replaced by two-steps algorithm (kinda):
    • first step is to prepare an array of format_part using format_string_compiler
    • second step is to create the same compiled format object as it was before (with recursion of concat<L, R>)
  • manual indexing with {0} and automatic indexing with {name} usage works exactly as it works in runtime API (thanks to format_string_compiler)
  • named replacement fields are compiled into their own compiled_format types (runtime_named_field and runtime_named_spec_field), but these types just hold name or name+specs to be able to use write() function or to create formatter for specified specs
  • single *index argument kind divided into index_auto and index_manual to have meta info about how the argument was created, maybe it's not the best solution, but this meta info needed for proper spec_field creation
  • tests added

I was afraid of making compilation time significantly longer by using the current approach, so I made a very straightforward test by compiling compile-test.cc with the same tests (master version of this file) several times, here are average times (mean ± σ, big thanks to hyperfine):

Compiler master this PR
clang++-11 2.831 s ± 0.016 s 2.894 s ± 0.019 s
clang++-11 -g 3.373 s ± 0.026 s 3.425 s ± 0.022 s
clang++-11 -O3 -DNDEBUG 10.632 s ± 0.016 s 10.601 s ± 0.042 s
g++-10 4.728 s ± 0.024 s 4.819 s ± 0.023 s
g++-10 -g 5.299 s ± 0.024 s 5.403 s ± 0.032 s
g++-10 -O3 -DNDEBUG 11.787 s ± 0.028 s 11.978 s ± 0.033 s

So, there is some compilation time increase (in most cases), but I'm not sure how bad it is.

@alexezeder alexezeder marked this pull request as draft January 25, 2021 14:12
@alexezeder alexezeder marked this pull request as ready for review January 25, 2021 14:22
@alexezeder
Copy link
Contributor Author

Hmm... looks like there is a problem with parsing specs for custom types because the default specs parser (parse_format_specs() function) is invoked even for custom types in format_string_compiler::on_format_specs(). For example:

struct test_struct {
  int value;
};

template <> struct formatter<test_struct> {
  enum class output_type { two, four } type{};

  constexpr auto parse(format_parse_context& ctx) {
    auto it = ctx.begin(), end = ctx.end();
    while (it != end && *it != '}') {
      ++it;
    }
    auto spec = string_view(ctx.begin(), it - ctx.begin());
    if (compare(spec, "t")) { // custom compare() cause fmt::string_view comp operators are not constexpr
      type = output_type::two;
    } else if (compare(spec, "four symbols, please")) {
      type = output_type::four;
    }
    return it;
  }

  template <typename FormatContext>
  auto format(const test_struct& object, FormatContext& ctx) const {
    return format_to(ctx.out(), type == output_type::two ? "{:>2}" : "{:>4}", object.value);
  }
};

int main() {
  format("{:t}", test_struct{42}); // OK - "42"
  format("{:four symbols, please}", test_struct{42}); // OK - "  42"
  format(FMT_COMPILE("{:t}"), test_struct{42}); // OK - "42", cause 't' works as type in default specs parser
  format(FMT_COMPILE("{:four symbols, please}"), test_struct{42}); // compilation error
  return 0;
}

I will create a new variant of format_string_compiler then, keeping the old one untouched.

But for now, I'm converting this PR to a draft.

@alexezeder alexezeder marked this pull request as draft January 26, 2021 14:57
@alexezeder
Copy link
Contributor Author

alexezeder commented Jan 28, 2021

Despite the fact that I found a workaround for the problem described above, it's so bad that I would probably take the initial approach with keeping that recursion of compile_format_string() + parse_tail() functions to produce compiled format structure.

The workaround is pretty straightforward - pass a tuple-like object which will have optional formatters for each passed argument.
Something like tuple<optional<formatter<int>>, optional<formatter<float>>> for int, float arguments. This tuple-like object will be filled for each spec'ed argument in format_string_compiler::on_format_specs() to be later used in combining of concat-based compiled format.

Should I say that it looks really bad? 😏

Since I think it can be done better (with a custom string pass, as it is right now in master) I am closing this PR.

@alexezeder alexezeder closed this Jan 28, 2021
@alexezeder alexezeder deleted the feature/support_full_syntax_in_compile_time_api branch February 7, 2021 20:06
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.

None yet

1 participant