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

Simplify a copying the fill from basic_specs #4290

Merged
merged 1 commit into from
Jan 5, 2025

Conversation

phprus
Copy link
Contributor

@phprus phprus commented Jan 4, 2025

Simplify code.

Fix for #4289

@phprus
Copy link
Contributor Author

phprus commented Jan 4, 2025

@ja11sop , test this PR, please.

@ja11sop
Copy link

ja11sop commented Jan 5, 2025

I just ran against this PR and all looks good

@phprus
Copy link
Contributor Author

phprus commented Jan 5, 2025

@vitaut please review this PR.

@vitaut
Copy link
Contributor

vitaut commented Jan 5, 2025

I wonder why the issue reported in #4289 hasn't been caught by existing tests? It might be worth adding a regression test.

@vitaut vitaut merged commit 2c3a569 into fmtlib:master Jan 5, 2025
45 checks passed
@phprus
Copy link
Contributor Author

phprus commented Jan 5, 2025

@vitaut

I'm guessing that a previous version of the code might have corrupted memory somewhere earlier.

I don't see the root of case for this error, but I think move the fill copying into the basic_specs class as a member function is a nice simplification of the logic.

The code from PR is free of any possibility of memory corruption.

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