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

Re-enable signed/unsigned mismatch warnings #8505

Open

Conversation

@CookiePLMonster
Copy link
Contributor

CookiePLMonster commented Dec 2, 2019

Fixes all instances of this warning, so it can be re-enabled. This opens the way for more signed/unsigned typecast removals.

@CookiePLMonster CookiePLMonster force-pushed the CookiePLMonster:fix-signed-unsigned-mismatches branch from 06d5ec2 to d3c0541 Dec 2, 2019
@@ -252,7 +252,7 @@ void DSPEmitter::addpaxz(const UDSPInstruction opc)
get_long_acx(sreg, tmp1);
MOV(64, R(RDX), R(tmp1));
// s64 res = prod + (ax & ~0xffff);
MOV(64, R(RAX), Imm64(~0xffff));
MOV(64, R(RAX), Imm64(~0xffffull));

This comment has been minimized.

Copy link
@lioncash

lioncash Dec 2, 2019

Member

We generally use the uppercased variant of the literals to prevent l being mistaken for 1 in some fonts. Ditto for other occurrences.

@@ -82,7 +84,7 @@ void DSPEmitter::andcf(const UDSPInstruction opc)
OR(16, sr_reg, Imm16(SR_LOGIC_ZERO));
FixupBranch exit = J();
SetJumpTarget(notLogicZero);
AND(16, sr_reg, Imm16(~SR_LOGIC_ZERO));
AND(16, sr_reg, Imm16(~SR_LOGIC_ZERO & 0xFFFF));

This comment has been minimized.

Copy link
@lioncash

lioncash Dec 2, 2019

Member

This is still technically resulting in a signedness conversion (and a truncation), this is just off by default in MSVC (C4365). This results in an int which is truncated down to a u16. You may just want to do:

Imm16(u16(~SR_LOGIC_ZERO)))

rather than masking, since it makes the conversion more explicit and also addresses that warning category as well.

This comment has been minimized.

Copy link
@CookiePLMonster

CookiePLMonster Dec 3, 2019

Author Contributor

Ugh, my bad - maybe I'll even enable this warning, because it looks fairly useful. That said, is there absolutely no way to fix it without any typecasting? Function style cast looks "nice" here, but it's still a cast.

This comment has been minimized.

Copy link
@lioncash

lioncash Dec 3, 2019

Member

In this case, type casting would be necessary.

This comment has been minimized.

Copy link
@CookiePLMonster

CookiePLMonster Dec 3, 2019

Author Contributor

Type casting like you suggested generates warning C4310 - cast truncates constant value. Maybe the way I have it is preferable after all.

This comment has been minimized.

Copy link
@CookiePLMonster

CookiePLMonster Dec 3, 2019

Author Contributor

Given how even Microsoft headers generate C4365, I would just ignore that fact.

Fixes all instances of this warning, so it can be re-enabled.
This opens the way for more signed/unsigned typecast removals.
@CookiePLMonster CookiePLMonster force-pushed the CookiePLMonster:fix-signed-unsigned-mismatches branch from d3c0541 to 6de7521 Dec 4, 2019
@CookiePLMonster CookiePLMonster requested a review from lioncash Dec 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.