[badminton1]

In flowchart decision points, it's clearer to make "yes" co-linear and "no" orthogonal.

Likewise I prefer to structure cyclomatic complexity like this by doing something like

   if not <condition 1>
      return
   else if not <condition 2>
      return

   <logic>

Rather than

    if <condition1>
        if <condition2>
            if <condition 3>
                ...

[richardwhiuk]

Early returns here will require duplicated free() calls, which makes it hard to prevent leaks.

This coding style is standard practice for Win32 API development.

[int_handler]

> This coding style is standard practice for Win32 API development.

Do you have a public style guide that indicates this?

I worked at Microsoft, and while there are teams that (very unfortunately) went with this "diamond" style of error handling, code written in the CNF (Cutler Normal Form) style, such as the kernel code, use gotos and a central cleanup point at the end of the function, which is also the same style used by the Linux kernel: https://github.com/torvalds/linux/blob/master/Documentation/...

The "diamond" style hinders readability, especially when you have many levels of indentation). While it may be a practice adopted by many people, that is does not necessarily mean that it is good style. Even Steve McConnell's Code Complete, a book published by Microsoft Press, recommends early returns over deeply nested if statements for error handling: https://books.google.com/books?id=LpVCAwAAQBAJ&lpg=PA764&ots...

Unfortunately, the Win32 API is full of technical debt, many of which cannot be repaid due to backwards compatibility reasons. Given it is likely most application code written against Win32 is in C++, the ideal approach would be to follow an object-oriented style and use destructors and RAII rather than cleanup functions, but that is not the world we live in.

The design of the Win32 API should not be used as guidelines for good coding practices or even API design. If you haven't already, I highly recommend reading "The Worst API Ever Made": https://mollyrocket.com/casey/stream_0029.html.

[int_19h]

The more typical coding style for raw Win32 is to have a block of CloseHandle/Free/... block at the end of the function, and goto it for early exit. Usually with some helper macros to easily write "if it failed, goto cleanup" one-liners.

[richardwhiuk]

You can't have a single cleanup (because subsequent calls use memory/handles from prior - otherwise you'd have a function wrapper), so it ends up getting really nasty - at which point, the logic feels clearer (and easier to match alloc and free) if you have indented blocks. The change I'd make to the code would be to have a shorter indent block.

  HRESULT hr;

  hr = DoFoo();

  if (!SUCCESS(hr))
  {
    goto exit;
  }

  hr = DoBar();

  if (!(SUCCESS(hr))
  {
    goto cleanup_foo;     
  }

  hr = DoBar();

  if (!(SUCCESS(hr))
  {
    goto cleanup_bar;     
  }

  hr = DoBaz();

  if (SUCCESS(hr))
  {
    CleanupBaz();
  }

  cleanup_bar:
    CleaupBar();

  cleanup_foo:
    CleanupFoo();

  exit:

  return hr;

[int_19h]

> You can't have a single cleanup (because subsequent calls use memory/handles from prior

Why not? You just initialize them all to NULL (or INVALID_HANDLE_VALUE etc, as appropriate). And then in your cleanup block, you check whether each was initialized, and clean it up. So long as you consistently do this in reverse order of their initialization, it works.