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

Changed format_windows_error to not need LocalFree #285

Merged
merged 2 commits into from
Mar 6, 2016

Conversation

mwinterb
Copy link
Contributor

@mwinterb mwinterb commented Mar 3, 2016

This is for non-'Desktop' applications that have a more limited collection of functions.
Addresses #280.

This is for non-'Desktop' applications that have a
more limited collection of functions.
@vitaut
Copy link
Contributor

vitaut commented Mar 3, 2016

Thanks for the PR, but why not free the buffer with HeapFree (GetProcessHeap(), allocatedMessage) as suggested in https://msdn.microsoft.com/en-us/library/windows/desktop/ms679351(v=vs.85).aspx instead of increasing the buffer size in a loop?

@mwinterb
Copy link
Contributor Author

mwinterb commented Mar 3, 2016

The documentation for the define indirectly calls out that it wasn't available for "Windows Store apps" for Windows 8 and 8.1 and the latest Windows SDK only defines it for "desktop family" applications. #280 also pointed out that FORMAT_MESSAGE_ALLOCATE_BUFFER was not defined. It not being defined for non-desktop family applications if Windows 10 is being targeted is probably a Windows header file bug, but cppformat would still have to deal with old SDKs even if that gets fixed.

So, basically, just to maximize compatibility with as many different Windows "versions" as possible.

I need to also amend this PR with a test to make sure that TBS_E_PROVISIONING_NOT_ALLOWED works consistently with directly calling FormatMessageW(ALLOCATE) since that error code forces the retry, I just wasn't aware of that case when I started.

@vitaut
Copy link
Contributor

vitaut commented Mar 3, 2016

Fair enough. I'm not quite sure what TBS_E_PROVISIONING_NOT_ALLOWED is about but I'll wait for you to update the PR.

@mwinterb
Copy link
Contributor Author

mwinterb commented Mar 3, 2016

The message text for TBS_E_PROVISIONING_NOT_ALLOWED is more than 500 characters long, so it'll just be to verify that the buffer is too small case is handled correctly.

vitaut added a commit that referenced this pull request Mar 6, 2016
Changed format_windows_error to not need LocalFree
@vitaut vitaut merged commit 6883d6e into fmtlib:master Mar 6, 2016
@vitaut
Copy link
Contributor

vitaut commented Mar 6, 2016

Merged, thanks!

@mwinterb mwinterb deleted the winerror_winu branch March 18, 2016 17:18
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.

2 participants