User Details
- User Since
- Jul 25 2018, 10:28 AM (270 w, 4 d)
Dec 3 2022
Overall looks good but we need to fix escaping in the standard and the implementation before this ships. Also a bunch of minor(ish) comments inline.
Sep 17 2022
One minor comment inline otherwise looks great. Much more readable than template operator()!
Jul 30 2022
Next I want to look at the std::back_inserter<std::string>.
Jul 29 2022
I'm actually surprised how fast this issue went through LWG :-)
Jul 27 2022
One check less, yay!
Nice improvement but I'd still recommend exploring direct writes to contiguous containers as discussed on the other diff.
Jul 16 2022
Jul 10 2022
Nice improvement! It would be good to clarify in the standard that omitting parse is allowed for user-defined types. Could you open an LWG issue?
Jul 9 2022
Mostly looks good but at least one output doesn't look right which suggests an error in width estimation. Also a few nits inline.
Jun 25 2022
Jun 19 2022
LGTM
Jun 12 2022
Jun 8 2022
Jun 6 2022
Jun 5 2022
Looks great! My main suggestion is to rename __parser into something more appropriate (see an inline comment) and possibly separate the actual parsing logic from the parser output.
Jun 4 2022
In general looks good but I would strongly recommend not to instantiate all of formatting path just to validate width/precision (see an inline comment).
May 15 2022
LGTM, just one minor comment inline.
May 8 2022
Apr 22 2022
LGTM
Apr 16 2022
Looks much better, thanks. A few comments inline.
Apr 11 2022
Apr 8 2022
Apr 1 2022
LGTM
Can you explain why you dislike this approach?
Mar 27 2022
You could do much better by having a single array and storing the type alternatives for the common case of small(ish) number of arguments in an integer. This is essentially what {fmt} does. I think the current implementation is based on Microsoft's which I'd strongly discourage.
Mar 9 2022
Jan 24 2022
Great work, @Mordante! Can't wait to use more of std::format on gobolt.
Jan 23 2022
LGTM
Jan 17 2022
Jan 16 2022
Same as D110495:
As commented, I'm very skeptical about this approach because it has extra buffering for common cases but I defer to libc++ maintainers the decision whether they want to land this as is or optimize buffering now.
Overall looks good, just a bunch of minor(ish) comments inline.
Jan 12 2022
Jan 1 2022
Dec 24 2021
As commented inline, to_chars is not a great API for implementing this because you have to do extra work to reparse the output. But it's probably OK for the initial implementation.
Dec 23 2021
LGTM
LGTM
Dec 22 2021
Overall looks good but I think there is a potential buffer overflow in format (see inline comment).
Dec 19 2021
But I consider that an issue in to_chars and not in <format>.
All parts of the paper have been implemented
Dec 18 2021
LGTM
Nov 26 2021
Nov 25 2021
In general looks good, just a few minor comments inline. However, the discussion in earlier diffs may affect __formatted_size_buffer API.
Nov 24 2021
Nov 21 2021
LGTM provided that the extra copy and buffering is eliminated in the follow-up diffs. Not directly related to this diff but I recommend doing something about the default codegen (see the inline comment).
Nov 17 2021
Nov 12 2021
Nov 11 2021
Nov 10 2021
Nov 7 2021
Oct 31 2021
Character-by-character output, especially type erased one introduced in D110494 doesn't seem efficient. As commented on the the previous diff the buffer and corresponding output iterator should provide access to a contiguous buffer and type erase the reallocation, not character output.
Since there's no guarantee every container's insert behaves properly
Oct 24 2021
Oct 19 2021
Oct 18 2021
Oct 17 2021
I'm very surprised that this is the solution you went with. Indirect function call to write each character individually will likely be slow in practice compared to a contiguous buffer and a bulk copy. Please post benchmark results.
Now that the format context is almost always format_context, it might be worth adding [[clang::preferred_name(format_context)]] to the basic_format_context template.
Minor comment inline, otherwise LGTM.
Sep 5 2021
Aug 8 2021
LGTM
LGTM
Aug 7 2021
Two minor comments inline, otherwise LGTM.
Jul 28 2021
Jul 25 2021
Looks good, thanks for addressing the comments.
Jul 18 2021
LGTM
Jul 17 2021
Some nits inline, otherwise LGTM.