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 basic array safety functions and backwards-compatible result type #3805

Merged
merged 4 commits into from
Jan 16, 2024

Conversation

ThePhD
Copy link
Contributor

@ThePhD ThePhD commented Jan 13, 2024

This pull request follows-up on #3756 by doing one part of what was asked; just array overloads to cover that case. A separate PR will handle the enhanced functionality for range-based calls to format_to (and a potentially new API call) in fmt/range.h (and not in core).

Unlike the general-purpose sketch in #3756, this one is meant to be merged.

@vitaut
Copy link
Contributor

vitaut commented Jan 13, 2024

Thanks for the PR! Looks like there are some build failures.

include/fmt/base.h Outdated Show resolved Hide resolved
@ThePhD ThePhD force-pushed the thephd/output-ranges-arrays branch from 8925e14 to c24a8cd Compare January 14, 2024 06:18
@ThePhD
Copy link
Contributor Author

ThePhD commented Jan 14, 2024

I think (?) the build failures should go away soon. Only thing I haven't done here is call std::forward for the new forwarding references. I don't really want to #include <utility> after all the work you went through to reduce compilation times in base.h. If I can remember how to write a macro-only FWD(x), I'll try to do that and then apply it to all the places where I had to take OutputIt&&.

Look okay otherwise?

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.

Overall looks good but let's limit the changes to the main API in fmt/base.h. Once we get usage experience and happy with the new API we could apply the change to other places.

Only thing I haven't done here is call std::forward for the new forwarding references. I don't really want to #include after all the work you went through to reduce compilation times in base.h. If I can remember how to write a macro-only FWD(x), I'll try to do that and then apply it to all the places where I had to take OutputIt&&.

The macro is described here: https://www.foonathan.net/2020/09/move-forward/. {fmt} had something like that at some point but it was removed as we got rid of most forwarding.

@phprus
Copy link
Contributor

phprus commented Jan 14, 2024

I think it's better to add a Char (&out)[Size] overload for the format_to_n functions.
This will not require changing the return type for the format_to function.

@ThePhD
Copy link
Contributor Author

ThePhD commented Jan 14, 2024

I think it's better to add a Char (&out)[Size] overload for the format_to_n functions.
This will not require changing the return type for the format_to function.

If this is a deal breaker, I would rather remove the return type than change format_to's behavior back to the old one.

It does not make sens e to change format_to_n as well, unless you want additional safety by checking the size passed in versus the size of the array and take whichever is smaller. (I'm sure this is also better for safety as well, too, and I can do that in a different PR.)

@vitaut
Copy link
Contributor

vitaut commented Jan 14, 2024

I agree with @ThePhD that we need to change format_to because otherwise we won't get improved safety.

@phprus
Copy link
Contributor

phprus commented Jan 14, 2024

@vitaut
I suggested an overload:

template <typename Char, size_t n, typename... T>
auto format_to_n(Char (&out)[n], format_string<T...> fmt, T&&... args)

without duplicating n.
So that the function name explicitly indicates that there is a limit on the length of the result.

@ThePhD ThePhD force-pushed the thephd/output-ranges-arrays branch from c24a8cd to acc60e6 Compare January 14, 2024 16:45
@ThePhD
Copy link
Contributor Author

ThePhD commented Jan 14, 2024

I changed it to just base.h. I'll try to workshop something for format_to_n later. The linux build started breaking but it seems unrelated to the changes I made and instead what's currently being worked on in master?

— 💬 "My bad; botched copy-paste job"
@ThePhD
Copy link
Contributor Author

ThePhD commented Jan 14, 2024

Oh, nevermind. I made a mistake; should be fixed with the latest push now.

@vitaut
Copy link
Contributor

vitaut commented Jan 14, 2024

If you plan to make changes to format_to_n, please do them in a separate PR.

@ThePhD
Copy link
Contributor Author

ThePhD commented Jan 14, 2024

Yes, any changes to format_to_n will be made separately.

@ThePhD ThePhD requested a review from vitaut January 14, 2024 18:50
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.

A few more comments and please add a test case to base-test.

include/fmt/base.h Show resolved Hide resolved
include/fmt/base.h Outdated Show resolved Hide resolved
include/fmt/base.h Outdated Show resolved Hide resolved
Comment on lines 2832 to 2836
FMT_CONSTEXPR operator OutputIt&() & noexcept { return out; }
FMT_CONSTEXPR operator const OutputIt&() const& noexcept { return out; }
FMT_CONSTEXPR operator OutputIt&&() && noexcept {
return static_cast<OutputIt&&>(out);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need all three overloads and why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could make a const-only version of this function that just unconditionally returns a copy. It'd be cheap for all the cases we care about, but if later functionality is added to do e.g. real output_range that has a move-only iterator, then it'll start being a compilation break to use this type.

There is also the case that for real output_range types, you'd want to return a std::ranges::subrange or a sufficient mockup of one, so it might be worth it to invest in making a real fmt::detail::subrange for the return value.

include/fmt/base.h Outdated Show resolved Hide resolved
include/fmt/base.h Outdated Show resolved Hide resolved
@vitaut vitaut merged commit 13ec66b into fmtlib:master Jan 16, 2024
41 checks passed
@vitaut
Copy link
Contributor

vitaut commented Jan 16, 2024

Merged, thanks!

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