concerning clang warnings

[Morilli]

Compiling the core with clang reveals some interesting warnings. Most of these seem irrelevant, but here's two that indicate actual incorrect logic:

../../src/device/gb/gb_cart.c:243:80: warning: operator '?:' has lower precedence than '|'; '|' will be evaluated first
      [-Wbitwise-conditional-parentheses]
  243 |         gb_cart->rom_bank = (gb_cart->rom_bank & ~UINT8_C(0x1f)) | (bank == 0) ? 1 : bank;
      |                             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
../../src/device/gb/gb_cart.c:243:80: note: place parentheses around the '|' expression to silence this warning
  243 |         gb_cart->rom_bank = (gb_cart->rom_bank & ~UINT8_C(0x1f)) | (bank == 0) ? 1 : bank;
      |                                                                                ^
      |                             (                                                 )
../../src/device/gb/gb_cart.c:243:80: note: place parentheses around the '?:' expression to evaluate it first
  243 |         gb_cart->rom_bank = (gb_cart->rom_bank & ~UINT8_C(0x1f)) | (bank == 0) ? 1 : bank;
      |                                                                                ^
      |                                                                    (                     )

This looks like an actual bug: a bitwise OR with the result of bank == 0 is executed, which makes no sense. Here's a patch for what I believe to be the correct logic:

diff --git a/src/device/gb/gb_cart.c b/src/device/gb/gb_cart.c
index a6414171..dc323535 100644
--- a/src/device/gb/gb_cart.c
+++ b/src/device/gb/gb_cart.c
@@ -240,7 +240,8 @@ static int write_gb_cart_mbc1(struct gb_cart* gb_cart, uint16_t address, const u
     /* 0x2000-0x3fff: ROM bank select (low 5 bits) */
     case (0x2000 >> 13):
         bank = value & 0x1f;
-        gb_cart->rom_bank = (gb_cart->rom_bank & ~UINT8_C(0x1f)) | (bank == 0) ? 1 : bank;
+        if (bank == 0) bank = 1;
+        gb_cart->rom_bank = (gb_cart->rom_bank & ~UINT8_C(0x1f)) | bank;
         DebugMessage(M64MSG_VERBOSE, "MBC1 set rom bank %02x", gb_cart->rom_bank);
         break;

The second one is:

../../src/device/dd/disk.c:432:40: warning: overlapping comparisons always evaluate to false [-Wtautological-overlap-compare]
  432 |         if (ipl_load_addr < 0x80000000 && ipl_load_addr >= 0x80800000) continue;
      |             ~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.

I don't have a patch for this, but clearly a comparison that always results in false is not right. Maybe this && was meant to be an ||? Or maybe a number was typo'd, I can't say for sure without additional information.

In any case both of these should be looked at.

[richard42]

thanks for the report, these should be fixed now.