Skip to content

Conversation

@dinomight
Copy link
Contributor

When dealing with dynamic named format args, need to account for nested named args when skipping the content of the replacement.

Fixes #4360

When dealing with dynamic named format args, need to account for
nested named args when skipping the content of the replacement.

Fixes fmtlib#4360
vitaut
vitaut previously requested changes Feb 26, 2025
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.

Thanks for the PR. I think the correct fix is to disable compile-time checks if there named arguments that are not known at compile time rather than trying to approximately parse.

@dinomight
Copy link
Contributor Author

That would disable arg ID checks also which seems to be an existing supported behavior when you mix static and dynamic named args. See how arg IDs are handled.

fmt/include/fmt/base.h

Lines 1693 to 1699 in bdbf957

FMT_CONSTEXPR auto on_arg_id(basic_string_view<Char> id) -> int {
for (int i = 0; i < NUM_NAMED_ARGS; ++i) {
if (named_args_[i].name == id) return named_args_[i].id;
}
if (!DYNAMIC_NAMES) on_error("argument not found");
return -1;
}

So are you suggesting to remove that as well? What about creating some default parse function that only verifies arg IDs within the format spec that it can verify at compile time?

@vitaut vitaut dismissed their stale review March 15, 2025 16:33

Thinking more of it, this does improve already existing skipping logic.

@vitaut vitaut merged commit 37e6474 into fmtlib:master Mar 15, 2025
45 checks passed
@vitaut
Copy link
Contributor

vitaut commented Mar 15, 2025

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.

Named arguments failing to compile with "unmatched '}' in format string"

2 participants