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

Improve StdioOutputStream with fwrite (#180) #188

Merged
merged 4 commits into from
Apr 6, 2021
Merged

Conversation

felipejinli
Copy link
Contributor

Replaced fputs with fwrite, added test and made spelling corrections

WIP help

(for reference, see the code changes)
On the testing side, I'm currently using String(validatingUTF8: <unsafe-mutable-pointer> to turn a UnsafeMutablePointer into a String. However, this seems to return a string that stops at the first 0 byte E.g. for a "hello\u{0} world" log it outputs hello only. NB. I have checked that fwrite correctly logs a message longer than "hello" through count but testing this is hard. @weissi @ktoso Have you got any suggestions?

Motivation:

If a 0 byte is logged, fputs would not output the content after the 0 byte making it harder to debug (see #180). fwrite uses a count argument so content after 0 byte can be logged.

Modifications:

Replaced fputs with the correct call to fwrite. Added helper internal func contiguousUTF8(_ string: String) -> String.UTF8View. Added testStdioOutputStreamWrite() to test ability to log content after 0 byte.

Result:

Improved debugging experience

@swift-server-bot
Copy link

Can one of the admins verify this patch?

8 similar comments
@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@felipejinli felipejinli changed the title [WIP] Improve StdioOutputStream with fwrite (#180) [WIP - HELP WANTED] Improve StdioOutputStream with fwrite (#180) Apr 4, 2021
@ktoso ktoso requested a review from weissi April 5, 2021 02:00
@ktoso
Copy link
Member

ktoso commented Apr 5, 2021

@swift-server-bot test this please

@weissi weissi changed the title [WIP - HELP WANTED] Improve StdioOutputStream with fwrite (#180) Improve StdioOutputStream with fwrite (#180) Apr 6, 2021
Copy link
Member

@weissi weissi left a comment

Choose a reason for hiding this comment

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

Thank you! This is pretty much perfect! Just a ! missing, then we can ship this.

Sources/Logging/Logging.swift Show resolved Hide resolved
@felipejinli felipejinli force-pushed the dev branch 2 times, most recently from d5ef3ba to 40b4a73 Compare April 6, 2021 10:37
@felipejinli
Copy link
Contributor Author

@weissi Makes sense - I've just incorporated the changes. It should work now!

@felipejinli felipejinli requested a review from weissi April 6, 2021 10:41
Copy link
Member

@weissi weissi left a comment

Choose a reason for hiding this comment

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

Awesome, thank you!

@weissi
Copy link
Member

weissi commented Apr 6, 2021

@swift-server-bot test this please

@weissi
Copy link
Member

weissi commented Apr 6, 2021

Sorry @felipejinli , could you run scripts/generate_linux_tests.rb to generate the Linux test list, that should make the soundness bot happy.

@felipejinli
Copy link
Contributor Author

@weissi done!

@weissi
Copy link
Member

weissi commented Apr 6, 2021

@swift-server-bot test this please

@weissi
Copy link
Member

weissi commented Apr 6, 2021

@swift-server-bot test this please

@weissi weissi merged commit b1d1c03 into apple:main Apr 6, 2021
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.

4 participants