fix(windows): cleanup pointer to int typecasts#3612
Conversation
Fixes #3084. This does two things: 1. Cleans up a bunch of places where we used to use `(int)` typecasts for pointer math, which was problematic. We now use `(INT_PTR)` per MSDN https://docs.microsoft.com/en-us/windows/win32/winprog64/rules-for-using-pointers and then cast that down to `(int)` where necessary, e.g. when storing string lengths which are never going to be more than a few hundred characters! Doing this explicitly helps to clarify that we are aware of the typecast and believe it to be safe. 2. Adds in some build infrastructure for future use of Coverity Scan https://scan.coverity.com/ which we plan to use for further code quality updates. I have submitted the project to Coverity and are now waiting for approval so we can check results. Once we have approval, I do plan to add this to the nightly build (we need to keep submissions under 3 builds/day). Note: I have not yet added Keyman Core (Windows) to this project, nor are we currently building Keyman Core (macOS) or Keyman for Linux, but we should consider adding those in future.
| q[0], q[1], q[2], | ||
| (temp ? (int)(temp-s->dpString) : 0));*/ | ||
| (temp ? (INT_PTR)(temp-s->dpString) : 0));*/ |
There was a problem hiding this comment.
Yeah, I know this code is currently commented out ... but I didn't want to skip it nor delete it at this point in time.
| # | ||
| # Build Keyman projects for coverity; call this with | ||
| # make test-coverity | ||
| # * Set environment variable COVBUILD to path to cov-build.exe |
There was a problem hiding this comment.
Would developers ever need to install coverity locally? If so, can you include notes in windows/src/README.md?
There was a problem hiding this comment.
Yep. I will be doing coverity as a separate PR once I've sorted out how it all works. This is groundwork for that.
|
I'm inclined to approve this. But I had a hard time keeping track when the pointer should be |
I understand the challenge. I have added the The two-part cast is technically unnecessary but helps to document that we are not accidentally downcasting where we shouldn't be. |
Fixes #3084.
This does two things:
Cleans up a bunch of places where we used to use
(int)typecasts for pointer math, which was problematic. We now use(INT_PTR)per MSDN https://docs.microsoft.com/en-us/windows/win32/winprog64/rules-for-using-pointers and then cast that down to(int)where necessary, e.g. when storing string lengths which are never going to be more than a few hundred characters! Doing this explicitly helps to clarify that we are aware of the typecast and believe it to be safe.Adds in some build infrastructure for future use of Coverity Scan https://scan.coverity.com/ which we plan to use for further code quality updates. I have submitted the project to Coverity and are now waiting for approval so we can check results. Once we have approval, I do plan to add this to the nightly build (we need to keep submissions under 3 builds/day).
Note: I have not yet added Keyman Core (Windows) to this project, nor are we currently building Keyman Core (macOS) or Keyman for Linux, but we should consider adding those in future.