Fix/3860 nested formatter precision - #4892
Conversation
fallenmi
left a comment
There was a problem hiding this comment.
Requesting changes at exact head ee0b93c72d2ecd933d24997304bf6e6de2488a0a.
The dynamic-precision path itself works when the formatter uses the new nested(value, ctx) overload: my exact-base oracle (21d4cc15c8a2a3536aab5623258ba3b3dae573ce) reports argument not found, while this head produces (1.00, 2.00). The focused head test also passes (format-test --gtest_filter=format_test.nested_formatter, 1/1, 9.5 s including configure/build).
There are two attributable blockers on this head:
- The repository's
g++-13 c++23 Release Sharedjob fails informat.h:4178: the new local aliasusing context = ...shadows the existingfmt::context, and the project promotes-Wshadowto an error. Please rename that alias (for example,nested_context_type). - The
clang-formatjob fails around the same specialization. I reproduced this with Apple clang-format 21 and the repository config: the exact base produces no diff, while the exact head rewrites lines 4171-4181. Please apply clang-format 21 to the touched code.
Please also drop unrelated commit d0d9adaf6ee72213ad2f9925b7056d5eb2890467 from this branch. Its C++20 module documentation change is already being reviewed separately in #4891 and is unrelated to the nested_formatter fix.
Disclosure: this review was prepared with Codex assistance; I verified the diff, exact SHAs, test outputs, and current CI logs before submitting it.
vitaut
left a comment
There was a problem hiding this comment.
Please remove unrelated commits and fix lint.
ee0b93c to
7dec989
Compare
Thank you for the review. Fixed and committed. Please re-review @fallenmi @vitaut |
7dec989 to
1fbe5d5
Compare
fallenmi
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 1fbe5d5a310819071d745f196ddfffe3d0fd4cc2 against current main b039e624c02ae473280156235217f9e02792b055 and synthetic merge 7b1eb1d338d717d46316ced2bb739dfe77a29f62.
My previous blockers are resolved: the shadowing alias is now nested_context_type, both g++-13 c++23 Release Shared and clang-format pass, and the branch rewrite removed the unrelated module-documentation commit. The context forwarding and dynamic width/precision handling are coherent, with coverage for automatic, positional, and named arguments plus output-iterator formatting.
The exact-head rollup is fully green: 47/47 checks across 8 suites and 7 workflows. I found no new blocker.
Disclosure: this review was prepared with Codex assistance; I verified the current refs, merge diff, review history, source changes, and CI through public GitHub data.
|
Closing in favor of cbb2641 but thanks for the PR. |
Fixes #3860
Dynamic width and precision specifications inside
nested_formatternowresolve argument IDs against the enclosing format context.
This adds: