MASSIVE warnings fix for bsnes

[knghtbrd]

The bsnes makefile doesn't actually enable warnings on platforms it claims to, but on Mac OS X, clang generates a TON of them anyway. Hundreds, actually, a couple of which were actually bugs. Since byuu has abandoned bsnes/higan, the best course seems to be submitting a patch here for them. We can propagate these to the other bsnes forks after a few people have tested this one to be sure that none of the warning fixes actually introduced any bugs.

[Alcaro]

+ #if !defined(APPLE) && !defined(clang)

I suspect that fails equally much on all Clang. I don't think ifdefing it to APPLE is a good solution.

+ if (offset < 0 && -offset > req_offset)

• req_offset = 0; //cannot seek before start of file
• else
• req_offset += offset;

Weird indentation. (GitHub seems to eat it in my post, but it shows up in the diff preview.)

uintmax_t req_offset = file_offset;

• if(req_offset < 0) req_offset = 0;

what was that guy thinking

+ default: /* Shouldn't happen? */ break;

Impossible to hit. Six of seven possible cases are handled, and the last one executes either continue or return a little earlier. Use NOT REACHED like the others.

carryout()

assigning to a function call. what kind of crazy shit is this

- while(regs.wai = true) {

• regs.wai = true;
• while(regs.wai) {

I am quite sure this changes observable behaviour. Change it back. Use double parens or something, like in libco.

+ if(bridge.timer && --bridge.timer == 0)

• ;

If you're sure there is a warning here, put --bridge.timer in the body of that one. If not, change it back.

// Return value on write isn't specified, let's fix that

We can't just stick in random values on code which may be reachable. Replace with // TODO: test on real hardware.

uint8 EpsonRTC::read(unsigned addr) { // NOT REACHED

Same here, this is reachable. regs.mdr is the standard undefiled value, but let's use 0 for now.

uint8 SA1::vbr_read(unsigned addr) { // NOT REACHED

Same here.

uint8 SuperFX::bus_read(unsigned addr) {

And here.

bool Controller::iobit() { if (port == Controller::Port1) return cpu.pio() & 0x40;

return cpu.pio() & 0x80;

Indent both paths equally.

void Video::update() { break; default: break;

Here too. Are you mixing spaces with size-4 tabs?

[knghtbrd]

+ #if !defined(APPLE) && !defined(clang)

I suspect that fails equally much on all Clang. I don't think ifdefing it to APPLE is a good solution.

Will adjust for all clang until/unless evidence surfaces to the contrary.

+ default: /* Shouldn't happen? */ break;

Impossible to hit. Six of seven possible cases are handled, and the last one executes either continue or return a little earlier. Use NOT REACHED like the others.

Current patch is better?

carryout()

assigning to a function call. what kind of crazy shit is this

I know what it does and why it does it, but agree that's not the right way to do it!

- while(regs.wai = true) {

• regs.wai = true;
• while(regs.wai) {

I am quite sure this changes observable behaviour. Change it back. Use double parens or something, like in libco.

Changed it back, although I'm not convinced that is defined behavior even with (()).

+ if(bridge.timer && --bridge.timer == 0)

• ;

If you're sure there is a warning here, put --bridge.timer in the body of that one. If not, change it back.

sfc/chip/armdsp/armdsp.cpp:42:42: warning: if statement has empty body [-Wempty-body]
  if(bridge.timer && --bridge.timer == 0);
                                         ^
sfc/chip/armdsp/armdsp.cpp:42:42: note: put the semicolon on a separate line to silence this warning

Obviously on second look, it should be (following byuu's "style"):

  if(bridge.timer) --bridge.timer;

The decrement happens unconditionally and the comparison result is ignored.

// Return value on write isn't specified, let's fix that

We can't just stick in random values on code which may be reachable. Replace with // TODO: test on real hardware.

That's shouldn't be a hardware issue, it's a "coding convention" issue. If bool write is false, a read is performed and a value returned. If write is true, no value is returned. But C++ never returns "no value" from a non-void function. Spec returns 0, and any compiler worth using assumes that you likely have a bug for very good reasons.

Basically compiler authors never imagined that someone would introduce confusing, difficult to maintain, and possibly buggy code trying to be "clever" and "efficient" (and lazy). sigh

I've made that more explicit.

uint8 EpsonRTC::read(unsigned addr) { // NOT REACHED

Same here, this is reachable. regs.mdr is the standard undefiled value, but let's use 0 for now.

I think I made the same mistake byuu did looking at addr &= 3;

[Alcaro]

void R65816::op_stp() {

• while(regs.wai = true) {
• regs.wai = true;
• while(regs.wai) { L op_io(); } } void R65816::op_wai() {
• regs.wai = true;
• while(regs.wai) {
• while((regs.wai = true)) { L op_io(); }

I ... don't think this is correct either. Looks like you managed to swap those two somehow.

That's shouldn't be a hardware issue, it's a "coding convention" issue.

Okay, you're right. I saw half a dozen of return memory_access(write, rom, addr, data);, but apparently 'write' is an argument and memory_access's return value is used correctly.

uint8 SA1::bitmap_read(unsigned addr) { // Nothing was returned here

This one is better off as NOT REACHED, like the others.

I'll merge this and fix those comments myself. It's easier for us both.