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

WinDef.DWORD getLow() & getHigh() using incorrect bit mask #624

Closed
imgx64 opened this issue Mar 28, 2016 · 4 comments
Closed

WinDef.DWORD getLow() & getHigh() using incorrect bit mask #624

imgx64 opened this issue Mar 28, 2016 · 4 comments
Labels

Comments

@imgx64
Copy link

imgx64 commented Mar 28, 2016

#128 was fixed by 245eb88 in 2012, but then 4218a07 reverted it with no explanation given. So I'm reopening it again.

Old bug description:

These two methods are using 0xFF as the bit mask when they should be using 0xFFFF. Each method returns a WORD, which is a 16-bit value. By using 0xFF only the lower byte is preserved.

New Code:

public WORD getLow() {
    return new WORD(longValue() & 0xFFFF);
}

public WORD getHigh() {
    return new WORD((longValue() >> 16) & 0xFFFF);
}

Old Code:

public WORD getLow() {
    return new WORD(longValue() & 0xFF);
}

public WORD getHigh() {
    return new WORD((longValue() >> 16) & 0xFF);
}
@dblock
Copy link
Member

dblock commented Mar 28, 2016

It would be useful to track down why/where it was reverted, otherwise needs a new PR and a test.

@dblock dblock added the bug? label Mar 28, 2016
@imgx64
Copy link
Author

imgx64 commented Mar 28, 2016

The "where" is 4218a07 , the "why" is probably by accident. The commit changed the whitespace on the whole file, which drowned the fact these two lines got reverted.

@dblock
Copy link
Member

dblock commented Mar 28, 2016

cc: @wolftobias

@matthiasblaesing
Copy link
Member

I created a PR (#683) that reverts the problem and introduces a test, that should prevent breakage in the future. The failing unittest on mac os x fails building the native part, so that result is ignored, unittests on windows work correctly.

If no veto is raised, the PR will be merged sometime next week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants