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

Update windows error reporting #127

Merged
merged 8 commits into from Sep 20, 2019

Conversation

@DanRStevens
Copy link
Member

DanRStevens commented Sep 19, 2019

This cleans up the GetLastErrorString function, and improves the interface slightly.


Potential future work:

There is still one use of a local buffer that must be freed with LocalFree, which is not handled in an exception safe manner. If the std::string methods throw, perhaps due to memory exhaustion, we would be leaking that memory. In PR #126, there was a LocalResource class that was added to handle such resources in an exception safe way using RAII. We could potentially use it here, though it seems it's going to need a little bit of work to add a few extra needed features.

The calls to GetLastErrorString could potentially do more of the string formatting. We probably don't actually need to pass the functionName parameter. That part of the formatting could be handled by the caller. It's also questionable if we really need the error code number in the error message. Removing both could simplify the method and make it much more highly targeted. Would love some feedback on that idea.

DanRStevens added 8 commits Sep 19, 2019
This removes the need to call `StringCchPrintf` to format the string.
That allows removing of the `<strsafe.h>` header import (requires
WindowsXP SP2 or greater). It also allows removing the `LocalAlloc`
call, and the associated pointer for the temporary string buffer.
This removes the need for one of the internal string conversions. It
also makes the API a little easier to use.
At this point, a better source of documentation is the help page for the
`FormatMessage` function.

The documentation for `FormatMessage` is currently found at:
https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-formatmessage
Copy link
Member

Brett208 left a comment

Changes look good. Thank you for working on cleaning this code up. My knowledge of the winapi is weak and working on it frustrates me.

Below is a screenshot of resulting error message. The %1 formatting artifact was preexisting, so feel free to merge without fixing.

ErrorMessage

I see no reason to keep the function name as an input argument. In fact that seems like a silly thing for me to have included in retrospect?

I like having the error code present. It encourages end users to lookup the error code online for more details. What if we made two versions of the function, one that returns the error code and one that does not? I wasn't sure what you were referring to using the function for in a more standardized manner so tough for me to comment more.

Feel free to merge and maybe post issues if you think it is worth it. Also happy to retest if you want to work on it more in this branch.

Thanks,
Brett

@DanRStevens

This comment has been minimized.

Copy link
Member Author

DanRStevens commented Sep 20, 2019

Eh, the function name argument was part of the original sample code that method was based on. Took me a while to even realize it wasn't that important, just part of the formatting. I initially though the function name might some sort of relevant to retrieving the actual error message. Kind of hard to know when using somewhat obscure Win API functions.

If we want to remove the parameter, we can do so in a future branch. I'll create an issue for it.

As for the "%1", I think that's related to the FORMAT_MESSAGE_IGNORE_INSERTS option. I suppose we could play with it if we wanted to.

@DanRStevens DanRStevens merged commit 16c8101 into master Sep 20, 2019
3 checks passed
3 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@DanRStevens DanRStevens deleted the updateWindowsErrorReporting branch Sep 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.