On 14/04/2022 19:14, Zebediah Figura wrote:
On 4/14/22 06:00, Giovanni Mascellani wrote:
Hi,
Il 08/04/22 19:00, Zebediah Figura ha scritto:
I don't think there's value in specifying a message for cases like this, and I'd rather not have to do so in new code. This applies to pretty much anywhere we have an assert() in a switch default case.
(Ideally we should avoid the 'default' keyword where possible, although in some cases this is difficult.)
vkd3d_unreachable() is written in a way such that you can pass it NULL to it for precisely this reason. I went adding messages for all present uses, but it is not mandatory to have one. Does this address your objection?
I noticed that, but even so, I don't think there's value in printing a message in any of these cases. A message only makes sense when we expect it to be printed, and we don't expect any of these messages to ever be printed. The only reason I added assert(0) was for code clarity, and I think it's quite clear why the default code paths are unreachable.
@@ -1542,12 +1534,10 @@ static void write_sm4_cast(struct hlsl_ctx *ctx, break; case HLSL_TYPE_BOOL: - /* Casts to bool should have already been lowered. */ - assert(0); - break; + vkd3d_unreachable("Casts to bool should have already been lowered");
This one I think should remain an assert. I can more easily see this getting triggered due to some forgotten implicit agreement. Hence we probably shouldn't tell the compiler this code path is unreachable.
Using an assert(!"string") or assert(0 && "string") construction (or defining our own macro) seems reasonable to me, though.
From the point of view of the compiler, assert(0) is not really different from vkd3d_unreachable(NULL): nothing tells the compiler that control will never ever reach it; if it happens, the compiler will print a message and abort(). The advantage of vkd3d_unreachable() is that the compiler won't try to tell you "Hey, you didn't write your return statement!" in places where it doesn't make any sense (because it's marked "noreturn").
As it's currently written, yes. However, I think we want to hook this up to __builtin_unreachable() if the latter is available.
What about something like DEFAULT_UNREACHABLE from winnt.h?