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

use memory_buffer to make color print behave atomic #1348 #1351

Merged
merged 4 commits into from
Oct 10, 2019

Conversation

tankiJong
Copy link
Contributor

I agree that my contributions are licensed under the {fmt} license, and agree to future changes to the licensing.

I did not have time to read the whole coding standard, but I did try to following the existing style.
Please do point out if there is any formating issue.

Thanks for your time :)

@tankiJong
Copy link
Contributor Author

I do not feel format_to( buf, "{}", format ); is the right way to put it..?

I try to format it and vprint the whole complete string but the complier complains when I type vprintf(f, buf.data());. Please inspire me the proper way to do it.

Thanks!

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.

include/fmt/color.h Outdated Show resolved Hide resolved
include/fmt/color.h Outdated Show resolved Hide resolved
include/fmt/color.h Outdated Show resolved Hide resolved
@tankiJong
Copy link
Contributor Author

Thanks for the feedback.
I will ping you again when I feel it's in shape and rebase the change into a single commit when you think it looks good:)

@tankiJong
Copy link
Contributor Author

@vitaut I am happy with the current version and seems CI is also happy now:D So feel free to review it.

@vitaut vitaut merged commit a82c1dc into fmtlib:master Oct 10, 2019
@vitaut
Copy link
Contributor

vitaut commented Oct 10, 2019

Merged, thanks!

}
buf.push_back(Char(0));
internal::fputs(buf.data(), f);
Copy link
Contributor

Choose a reason for hiding this comment

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

Although it is unusual, the result of formatting args may contain zero bytes, then this fputs will truncate it and will not reset the color.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, thanks for bring that up! But I don't think I understand what you mean... Could you give an example?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah.. Yes, that's a good point, in case there is a terminator in the string, that won't work.

orivej added a commit to orivej/fmt that referenced this pull request Oct 10, 2019
After fmtlib#1351 they became essentially the same.
orivej added a commit to orivej/fmt that referenced this pull request Oct 10, 2019
orivej added a commit to orivej/fmt that referenced this pull request Oct 10, 2019
orivej added a commit to orivej/fmt that referenced this pull request Oct 10, 2019
After fmtlib#1351 they became essentially the same.
vitaut pushed a commit that referenced this pull request Oct 11, 2019
After #1351 they became essentially the same.
orivej added a commit to orivej/fmt that referenced this pull request Oct 11, 2019
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