Vulkan-Tutorial icon indicating copy to clipboard operation
Vulkan-Tutorial copied to clipboard

Odd formatting for while loops

Open SaschaWillems opened this issue 5 months ago • 4 comments

We have a lot of places where while loops are written like this:

        while ( vk::Result::eTimeout == device.waitForFences( *inFlightFences[currentFrame], vk::True, UINT64_MAX ) )
            ;

This seems to be a short-hand version of while, hich is okay, but what looks odd is the line break and identation for the semicolon.

@gpx1000: Any reason the semicolon is on the next line and indented like that?

I'd prefer to clean up like this:

        while ( vk::Result::eTimeout == device.waitForFences( *inFlightFences[currentFrame], vk::True, UINT64_MAX ) );

Or maybe even remove the while? We already wait for a very long time (UINT64_MAX), the while loop would prob. never do anything as the TDR (at least on Windows) already would've reset the driver.

SaschaWillems avatar Aug 04 '25 19:08 SaschaWillems

I suspect it's fine to remove the while -- the spec is not actually fully clear on what UINT64_MAX means for vkWaitForFences in the dedicated section, however there is this in 5.2.3. Lost Device: Commands that wait indefinitely for device execution (namely vkDeviceWaitIdle, vkQueueWaitIdle, vkWaitForFences or vkAcquireNextImageKHR with a maximum timeout, and vkGetQueryPoolResults with the VK_QUERY_RESULT_WAIT_BIT bit set in flags) must return in finite time even in the case of a lost device, and return either VK_SUCCESS or VK_ERROR_DEVICE_LOST.

In other words, vkWaitForFences(..., timeout = UINT64_MAX) cannot return anything except SUCCESS or DEVICE_LOST, and so looping checking for timeout is unnecessary.

cforfang avatar Aug 06 '25 19:08 cforfang

Also several samples define FenceTimeout, but that is never used.

SaschaWillems avatar Aug 06 '25 20:08 SaschaWillems

It's also not consistent. Most samples use the while loop above, but others (like the profiles one) do this instead:

static_cast<void>(device.waitForFences({*inFlightFences[currentFrame]}, VK_TRUE, FenceTimeout));

Gotta admit, I'm not that much of a modern C++ fan, but the static_cast in front of a function call just looks wrong or at least very odd.

SaschaWillems avatar Aug 06 '25 20:08 SaschaWillems

This is mostly due to getting clang-lint to be happy with the latest version of clang-lint. waitForFences has a return value that isn't checked is the lint problem. Put it into a while loop that doesn't execute even the first frame of that while loop is a common macro type practice (i.e. you'll see a lot of macros that put statements like that into a while loop to make it easier to capture the log output). Putting the extra line break in is something clang-lint does automatically when combined with clang-format. Finally the function call to static_cast is a no-op so doesn't sacrifice anything. A more common C way of doing the same thing would be a macro something like: #macro DONT_USE((void*)...) Which naturally just places the c-style void cast infront of the unused variable or function. This is just a way to silence warnings explicitly.

That said, we should come up with a better standard practice and use it in the tutorial. Even though Charles didn't explicitly say it, I agree with what he didn't say, we should just check the result for success / fail as that call should block all on its' own.

gpx1000 avatar Aug 06 '25 20:08 gpx1000