Skip to content

Fix/3860 nested formatter precision - #4892

Closed
way4sahil wants to merge 1 commit into
fmtlib:mainfrom
way4sahil:fix/3860-nested-formatter-precision
Closed

Fix/3860 nested formatter precision#4892
way4sahil wants to merge 1 commit into
fmtlib:mainfrom
way4sahil:fix/3860-nested-formatter-precision

Conversation

@way4sahil

Copy link
Copy Markdown
Contributor

Fixes #3860

Dynamic width and precision specifications inside nested_formatter now
resolve argument IDs against the enclosing format context.

This adds:

  • Context forwarding for nested formatters.
  • Dynamic width handling.
  • Named and explicit argument support.
  • Tests for dynamic width, precision, padding, and static formatting.
  • A ChangeLog entry.

@way4sahil
way4sahil requested a review from vitaut as a code owner August 23, 2026 10:31

@fallenmi fallenmi left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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:

  1. The repository's g++-13 c++23 Release Shared job fails in format.h:4178: the new local alias using context = ... shadows the existing fmt::context, and the project promotes -Wshadow to an error. Please rename that alias (for example, nested_context_type).
  2. The clang-format job 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 vitaut left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please remove unrelated commits and fix lint.

@way4sahil
way4sahil force-pushed the fix/3860-nested-formatter-precision branch from ee0b93c to 7dec989 Compare August 30, 2026 16:48
@way4sahil

Copy link
Copy Markdown
Contributor Author

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:

1. The repository's `g++-13 c++23 Release Shared` job fails in `format.h:4178`: the new local alias `using context = ...` shadows the existing `fmt::context`, and the project promotes `-Wshadow` to an error. Please rename that alias (for example, `nested_context_type`).

2. The `clang-format` job 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.

Thank you for the review. Fixed and committed. Please re-review @fallenmi @vitaut

@way4sahil
way4sahil force-pushed the fix/3860-nested-formatter-precision branch from 7dec989 to 1fbe5d5 Compare August 30, 2026 17:28
@way4sahil
way4sahil requested review from fallenmi and vitaut August 30, 2026 17:41

@fallenmi fallenmi left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

@vitaut

vitaut commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Closing in favor of cbb2641 but thanks for the PR.

@vitaut vitaut closed this Sep 2, 2026
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.

Dynamic precision for nested_formatter does not work

3 participants