Bugfix #960
Bugfix #960
Conversation
minor bugfixes
GlobalMemoryStatus deprecated in x64
Some perfomance fixes and check return value of StringFromGUID2
|
@wendig0x, please fix the conflicts. |
|
@andreas-becker done |
There was a problem hiding this comment.
Halfway through the review I realized that some of the commits included into this PR were already separately merged in #950, but I decided to leave my comments in place, as they are still relevant in my opinion, even if late.
One general comment I have is that the commit messages are relatively low-information, so even though VC as a project doesn't have formal rules for those, I'd suggest to rework them in accordance with well-established recommendations (e.g. see here, but there are many summaries by now) and maybe consider splitting some of the commits to make them single-purpose/change, or more atomic.
|
@alt3r-3go Thanks for the great code review! I'm a little busy at the moment, I'll add comments and corrections later. |
| if (!AdjustTokenPrivileges(hThreadToken, FALSE, &tkp, 0, NULL, NULL)) | ||
| { | ||
| dwLastErr = GetLastError(); | ||
| goto cleanup; | ||
| } | ||
| if (GetLastError() == ERROR_NOT_ALL_ASSIGNED) | ||
| { | ||
| dwLastErr = ERROR_NOT_ALL_ASSIGNED; |
There was a problem hiding this comment.
These changes are not really necessary, the previous logic covered all the error paths and did it more concisely at that.
| @@ -14552,10 +14553,14 @@ static bool RunAsDesktopUser( | |||
| goto cleanup; | |||
There was a problem hiding this comment.
My previous comment about thise piece of code is still not resolved. dwLastError needs to be set in case of such an early exit, for the cleanup logic around SetLastError() to work properly.
| # For those using Visual Studio Code for development | ||
| .vscode/ |
There was a problem hiding this comment.
This file must not be removed from the version control. If you meant to keep your changes to it (in those vcxproj files, per my earlier review comment) locally, then just don't include it into your commit (skip during git add).
| @@ -15263,7 +15268,7 @@ void PasswordEditDropTarget::GotLeave(void) | |||
| DWORD PasswordEditDropTarget::GotEnter(void) | |||
| { | |||
| TCHAR szClassName[64]; | |||
| DWORD dwStyles; | |||
| LONG_PTR dwStyles; | |||
There was a problem hiding this comment.
Thanks for fixing this. There's another one in ::GotDrop further below, still not fixed.
| #endif // VC_COMREG |
There was a problem hiding this comment.
Please remove this irrelevant whitespace change to avoid polluting the change history.
| DWORD str_input = strlen(input); | ||
| DWORD str_mszDest = strlen(mszDest); |
There was a problem hiding this comment.
These two still have snake case (or even a mixed one for str_mszDest), so my previous comment about variable name casing is still actual. Moreover, the naming of those variables is still misleading (str_input suggests it's a "string of something we got as an input" or some such, and this is not the case). Using len prefix for the app name string length was good, so I'd suggest to do the same here.
In addition I did not repeat the comment for other variables and files for brevity in my previous review, but it's also applicable to other changes in this PR (e.g. str_len introduced in Dlgcode.c) - please correct them all.
|
@wendig0x |
Some performance fixes and check return value of StringFromGUID2