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

Output to stdout instead of calling OutputDebugString #27478

Closed
wants to merge 2 commits into from

Conversation

camelid
Copy link
Contributor

@camelid camelid commented Aug 1, 2020

Output to stdout instead of calling OutputDebugString since stdout redirects to OutputDebugString anyway and also to a log file.


  • There are tests for these changes OR
  • These changes do not require tests because ___

r? @jdm

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Aug 1, 2020
@@ -18,5 +18,5 @@ std::wstring format(const std::wstring &txt, Args... args) {
}

template <typename... Args> void log(const std::wstring &txt, Args... args) {
OutputDebugString((format(txt, args...) + L"\r\n").c_str());
std::cout << (format(txt, args...) + L"\r\n").c_str();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we want to use cout or would printf be better? I don't know C++ that well, but I know some C (and Rust of course!)

Copy link
Contributor

Choose a reason for hiding this comment

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

You'll be missing a #include here (iostream iirc).

… and I feel like the redirection doesn't work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Just added the include. It's weird how running ./mach build -d didn't give a warning. (Oh how I miss Rust's module system...)

Copy link
Contributor Author

@camelid camelid Aug 3, 2020

Choose a reason for hiding this comment

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

… and I feel like the redirection doesn't work.

Hmm, what do you mean? I was just implementing what @jdm suggested in #27441

EDIT: Oops, I had linked to this PR instead of the issue :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just realized that ./mach build -d might not have warned me because the header may have been included in a file that already had #include <iostream>...

Copy link
Contributor

Choose a reason for hiding this comment

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

./mach build -d doesn't build this code.

See https://github.com/servo/servo/wiki/Building-for-UWP

… and I feel like the redirection doesn't work.

Hmm, what do you mean? I was just implementing what @jdm suggested in #27441

I haven't investigated, but this doesn't appear to work.

@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably #29970) made this pull request unmergeable. Please resolve the merge conflicts.

@mrobinson
Copy link
Member

Closing this since it changes code that has now been removed.

@mrobinson mrobinson closed this Jul 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-awaiting-review There is new code that needs to be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make C++ log function use stdout
6 participants