fix "non-void function does not return a value" warning

[jg1uaa]

I think this might not be needed, but clang-13 on OpenBSD-7.4/amd64 says warning so fixed.

[ghaerr]

How about just using __builtin_unreachable() or return 0 rather than adding code that will never be executed (that also adds to the binary size)?

[jg1uaa]

debug_instruction() in cpu.c uses return value from disa() without check, so return 0 is not suitable. __builtin_unreachable() looks good and there is no problem with clang-13, maybe gcc.

[johnsonjh]

Maybe use something like ...

#if defined(__clang_version__) || defined(__GNUC__)
 __builtin_unreachable();
#endif

if you are worried about other compilers not groking it.

[dmsc]

Hi!

This is a bad warning by CLANG, as the code is obviously unreachable :-)

If you comment any of the cases above, gcc will correctly warn:

src/dis.c: In function ‘disa’:
src/dis.c:827:1: warning: control reaches end of non-void function [-Wreturn-type]
  827 | }
      | ^

So, IMHO, it is better to merge the following code:

diff --git a/src/dis.c b/src/dis.c
--- a/src/dis.c
+++ b/src/dis.c
@@ -823,4 +823,7 @@ const char *disa(const uint8_t *ip, uint16_t reg_ip, int segment_override)
     case 0xfe: return decode_b(ip, table_fe[(ip[1] & 0x38) >> 3], segment_override);
     case 0xff: return decode_ff(ip, segment_override);
     }
+#if defined(__clang_version__)
+    __builtin_unreachable();
+#endif
 }

What do you think?

[ghaerr]

So, IMHO, it is better to merge the following code

Unfortunately, even though I initially suggested it, __builtin_unreachable is not standard. Using it thus requires #ifdef code, which IMO is quite ugly, especially just to eliminate a compiler warning.

debug_instruction() in cpu.c uses return value from disa() without check, so return 0 is not suitable.

return 0 is still suitable, since that value will never actually be returned. But for those that worry nonetheless, perhaps just a return "" (or return "???") could be used - no ifdefs, no NULL dereferences, and very little code added.

[dmsc]

Hi!

I don't like the return 0 (or any return) because it will hide the warning if any of the case are actually missing, and I also feel that adding an IFDEF only to silence a warning from a buggy compiler is ugly.

agree to leave as it is?

[tkchia]

Hello all,

I am surprised that clang issues a warning even if all the possibilities for *ip are in fact covered.

Some possible ways I could think of to ward off the warning, if we really want to:

• Instead of __builtin_unreachable(); maybe say something like assert(false); return 0; (?) This is very much in the C standard, and it can hopefully be optimized away, if the compiler can deduce it will never happen.
• Or we could say return decode_databyte(ip, "DB"); — this is the way that unrecognized opcodes are handled inside the switch anyway.

Thank you!

[ghaerr]

Hello!

While @tkchia's idea of using assert(false) is pretty good at removing any code generated, providing that assert is always a macro and passing a constant false calls a routine with __attribute__((__noreturn)), @dmsc's concern that should a switch case be inadvertently be removed and no warning produced seems to me more important than silencing a compiler warning that shouldn't be produced in the first place.

I think this might not be needed, but clang-13 on OpenBSD-7.4/amd64 says warning

agree to leave as it is?

Perhaps some things are better left not "fixed"?

[jg1uaa]

/*NOTREACHED*/ and return show(ip, "bug?"); method shows clearly unreach code and displays when malworked. And, no compiler specific code.

But,

It is obvious that disa() needs no return (something) at last of code. So we can say that this is compiler issue.

Through this discussion, I think this PR is not required. I vote +1 for "leave as it is".

[dmsc]

Ok, closing with three votes :)