Coding when you are using an OS or library that requires a series of calls that build on each other to get the final result requires a set of checks to make sure each stage is successful. If some of the stages also require it partner call to free/close/destroy the items, then the programming logic requires you to be careful in how you tear down the items if there is an error in the middle of the process.
This can be coded several ways, and this post discusses a series of approaches in C++. The code selected for the example is performing Windows OS calls. These are done at the lowest API level without any other library or framework. The code has three places where tear down is required. The flow is complex enough to stop a simple reorganisation of the code to perform the same task. There is no addition error management present so make it easier to read. It is a fully working program and it gets a list of USB devices that the program is allowed access to.
Indent
This is the old school – direct way. You can see how and why it was originally taught. The flow is clear and all in the expected order and uses the simplest language structure (if/else). However, when the logic grows this can get out of hand. The tear down is a long way from the construction so it can end up in the wrong place when the code is edited later on. Also, when testing, the error cases are hard to check. It may look visually correct but it hard to test all the possible logic paths.
#include <windows.h>
#include <hidsdi.h> // Link with: hib.lib
#include <setupapi.h> // Link with: setupapi.lib
#include <stdio.h>
int main()
{
BOOL success{ TRUE };
SP_DEVICE_INTERFACE_DATA info{ sizeof(SP_DEVICE_INTERFACE_DATA) };
HIDD_ATTRIBUTES attributes{ sizeof(HIDD_ATTRIBUTES) };
DWORD index{ 0 };
GUID guid{ 0 };
DWORD size{ 0 };
PSP_DEVICE_INTERFACE_DETAIL_DATA_W details{ NULL };
HDEVINFO list{ NULL };
HANDLE handle{ INVALID_HANDLE_VALUE };
DWORD access{ GENERIC_READ | GENERIC_WRITE };
DWORD share{ FILE_SHARE_READ | FILE_SHARE_WRITE };
HidD_GetHidGuid(&guid);
list = SetupDiGetClassDevsW(&guid, NULL, NULL, DIGCF_PRESENT | DIGCF_INTERFACEDEVICE);
if (GetLastError()) return 0;
while (SetupDiEnumDeviceInterfaces(list, 0, &guid, index, &info))
{
index++;
size = 0;
SetupDiGetDeviceInterfaceDetailW(list, &info, NULL, 0, &size, NULL);
if (size != 0)
{
details = (PSP_DEVICE_INTERFACE_DETAIL_DATA_W)calloc(size, 1);
if (details != NULL)
{
details->cbSize = sizeof(SP_DEVICE_INTERFACE_DETAIL_DATA_W);
success = SetupDiGetDeviceInterfaceDetailW(list, &info, details, size, &size, NULL);
if (success)
{
handle = CreateFileW(
details->DevicePath, access, share, NULL, OPEN_EXISTING, 0, NULL);
if (handle != INVALID_HANDLE_VALUE)
{
success = HidD_GetAttributes(handle, &attributes);
if (success)
{
printf("Device attributes: VID=%d PID=%d Version=%4.4X\n",
attributes.VendorID, attributes.ProductID, attributes.VersionNumber);
}
CloseHandle(handle);
}
}
free(details);
}
}
}
SetupDiDestroyDeviceInfoList(list);
return 0;
}
Goto
This is the classic solution for the C language, and this is still used today in the Linux kernel. The objectives are clear, keep the main line of logic progressing as clearly as possible. Put the error paths later in the code so that it does not interfere with the visual reading of the main code. It can also be argued that the final code will typically only run through the successful path and that it is good to keep that code as localised as possible.
However, the same issues with the intending method apply here. When the code is updated, it is very hard to make sure all the logic paths are kept in sync. There is also now such a culture of ‘Never Use Gotos’ that this coding style is very hard to advocate.
#include <windows.h>
#include <hidsdi.h> // Link with: hib.lib
#include <setupapi.h> // Link with: setupapi.lib
#include <stdio.h>
int main()
{
BOOL success{ TRUE };
SP_DEVICE_INTERFACE_DATA info{ sizeof(SP_DEVICE_INTERFACE_DATA) };
HIDD_ATTRIBUTES attributes{ sizeof(HIDD_ATTRIBUTES) };
DWORD index{ 0 };
GUID guid{ 0 };
DWORD size{ 0 };
PSP_DEVICE_INTERFACE_DETAIL_DATA_W details{ NULL };
HDEVINFO list{ NULL };
HANDLE handle{ INVALID_HANDLE_VALUE };
DWORD access{ GENERIC_READ | GENERIC_WRITE };
DWORD share{ FILE_SHARE_READ | FILE_SHARE_WRITE };
HidD_GetHidGuid(&guid);
list = SetupDiGetClassDevsW(&guid, NULL, NULL, DIGCF_PRESENT | DIGCF_INTERFACEDEVICE);
if (GetLastError()) return 0;
while (SetupDiEnumDeviceInterfaces(list, 0, &guid, index, &info))
{
index++;
size = 0;
SetupDiGetDeviceInterfaceDetailW(list, &info, NULL, 0, &size, NULL);
if (size == 0) continue;
details = (PSP_DEVICE_INTERFACE_DETAIL_DATA_W)calloc(size, 1);
if (details == NULL) continue;
details->cbSize = sizeof(SP_DEVICE_INTERFACE_DETAIL_DATA_W);
success = SetupDiGetDeviceInterfaceDetailW(list, &info, details, size, &size, NULL);
if (!success) goto freedetails;
handle = CreateFileW(details->DevicePath, access, share, NULL, OPEN_EXISTING, 0, NULL);
// If owned by OS or other apps will fail; just ignore these.
if (handle == INVALID_HANDLE_VALUE) goto freedetails;
success = HidD_GetAttributes(handle, &attributes);
if (!success) goto freeCloseHandle;
printf("Goto: Device attributes: VID=%d PID=%d Version=%4.4X\n",
attributes.VendorID, attributes.ProductID, attributes.VersionNumber);
freeCloseHandle:
CloseHandle(handle);
freedetails:
free(details);
}
SetupDiDestroyDeviceInfoList(list);
return 0;
}
Never Nester
Check out this nice video ‘Why You Shouldn’t Nest Your Code’ that talks about what a “Never Nester” is. See the video on YouTube by CodeAesthetic. This is the main reason this post exists as it was interesting to see what other options there were and to try to compare the pros and cons.
The core ideas behind ‘Never Nester’ are extraction into functions, early return, and using data structures to replace simple loops.
#include <windows.h>
#include <hidsdi.h> // Link with: hib.lib
#include <setupapi.h> // Link with: setupapi.lib
#include <stdio.h>
int main()
{
BOOL success{ TRUE };
SP_DEVICE_INTERFACE_DATA info{ sizeof(SP_DEVICE_INTERFACE_DATA) };
HIDD_ATTRIBUTES attributes{ sizeof(HIDD_ATTRIBUTES) };
DWORD index{ 0 };
GUID guid{ 0 };
DWORD size{ 0 };
PSP_DEVICE_INTERFACE_DETAIL_DATA_W details{ NULL };
HDEVINFO list{ NULL };
HANDLE handle{ INVALID_HANDLE_VALUE };
DWORD access{ GENERIC_READ | GENERIC_WRITE };
DWORD share{ FILE_SHARE_READ | FILE_SHARE_WRITE };
HidD_GetHidGuid(&guid);
list = SetupDiGetClassDevsW(&guid, NULL, NULL, DIGCF_PRESENT | DIGCF_INTERFACEDEVICE);
if (GetLastError()) return 0;
while (SetupDiEnumDeviceInterfaces(list, 0, &guid, index, &info))
{
index++;
size = 0;
SetupDiGetDeviceInterfaceDetailW(list, &info, NULL, 0, &size, NULL);
if (size == 0) continue;
details = (PSP_DEVICE_INTERFACE_DETAIL_DATA_W)calloc(size, 1);
if (details == NULL) continue;
details->cbSize = sizeof(SP_DEVICE_INTERFACE_DETAIL_DATA_W);
success = SetupDiGetDeviceInterfaceDetailW(list, &info, details, size, &size, NULL);
if (!success)
{
free(details);
continue;
}
handle = CreateFileW(details->DevicePath, access, share, NULL, OPEN_EXISTING, 0, NULL);
// If owned by OS or other apps will fail; just ignore these.
if (handle == INVALID_HANDLE_VALUE)
{
free(details);
continue;
}
success = HidD_GetAttributes(handle, &attributes);
if (!success)
{
CloseHandle(handle);
free(details);
continue;
}
printf("Device attributes: VID=%d PID=%d Version=%4.4X\n",
attributes.VendorID, attributes.ProductID, attributes.VersionNumber);
CloseHandle(handle);
free(details);
}
SetupDiDestroyDeviceInfoList(list);
return 0;
}
Never Nester Inner
This is doing the Never Nester approach in full. This was not done originally as it felt too ugly but seeing the added complexity of the tear down in the case above, this had to be tried as well. It is now much cleaner; however, it has forced a function that feels like it is just not needed. Feels a bit too ‘Clean Code’ perhaps?
#include <windows.h>
#include <hidsdi.h> // Link with: hib.lib
#include <setupapi.h> // Link with: setupapi.lib
#include <stdio.h>
void Inner(PSP_DEVICE_INTERFACE_DETAIL_DATA_W details)
{
BOOL success{ TRUE };
HANDLE handle{ INVALID_HANDLE_VALUE };
HIDD_ATTRIBUTES attributes{ sizeof(HIDD_ATTRIBUTES) };
DWORD access{ GENERIC_READ | GENERIC_WRITE };
DWORD share{ FILE_SHARE_READ | FILE_SHARE_WRITE };
handle = CreateFileW(details->DevicePath, access, share, NULL, OPEN_EXISTING, 0, NULL);
// If owned by OS or other apps will fail; just ignore these.
if (handle == INVALID_HANDLE_VALUE) return;
success = HidD_GetAttributes(handle, &attributes);
if (success)
{
printf("Device attributes: VID=%d PID=%d Version=%4.4X\n",
attributes.VendorID, attributes.ProductID, attributes.VersionNumber);
}
CloseHandle(handle);
}
int main()
{
BOOL success{ TRUE };
SP_DEVICE_INTERFACE_DATA info{ sizeof(SP_DEVICE_INTERFACE_DATA) };
DWORD index{ 0 };
GUID guid{ 0 };
DWORD size{ 0 };
PSP_DEVICE_INTERFACE_DETAIL_DATA_W details{ NULL };
HDEVINFO list{ NULL };
HidD_GetHidGuid(&guid);
list = SetupDiGetClassDevsW(&guid, NULL, NULL, DIGCF_PRESENT | DIGCF_INTERFACEDEVICE);
if (GetLastError()) return 0;
while (SetupDiEnumDeviceInterfaces(list, 0, &guid, index, &info))
{
index++;
size = 0;
SetupDiGetDeviceInterfaceDetailW(list, &info, NULL, 0, &size, NULL);
if (size == 0) continue;
details = (PSP_DEVICE_INTERFACE_DETAIL_DATA_W)calloc(size, 1);
if (details == NULL) continue;
details->cbSize = sizeof(SP_DEVICE_INTERFACE_DETAIL_DATA_W);
success = SetupDiGetDeviceInterfaceDetailW(list, &info, details, size, &size, NULL);
if (success) Inner(details);
free(details);
}
SetupDiDestroyDeviceInfoList(list);
return 0;
}
RAII
So why all this when we have C++ to help out. It feels like a nice approach to put these OS calls into their own object where the construction and tear down is clear and gathered together. It also allows for a nice way to report back the value that is needed for the next OS call. While this case can use the Windows OS Get/SetLastError, this approach would also allow for better and more explicit error reporting after the constructor is called.
The code is now all together and is well controlled. The final sequence of events looks very clean and readable. The downside is that this feels like a lot more code. Perhaps this would help if the code was being reused, but that would be like generating a framework like MFC.
#include <windows.h>
#include <hidsdi.h> // Link with: hib.lib
#include <setupapi.h> // Link with: setupapi.lib
#include <stdio.h>
class _CreateFileW
{
public:
_CreateFileW(wchar_t* path)
{
DWORD access{ GENERIC_READ | GENERIC_WRITE };
DWORD share{ FILE_SHARE_READ | FILE_SHARE_WRITE };
_handle = CreateFileW(path, access, share, NULL, OPEN_EXISTING, 0, NULL);
// Failure communicated by GetLastError() from CreateFileW()
}
~_CreateFileW()
{
if (_handle != INVALID_HANDLE_VALUE) CloseHandle(_handle);
}
HANDLE handle() { return _handle; }
private:
HANDLE _handle{ INVALID_HANDLE_VALUE };
};
class _SetupDiGetClassDevsW
{
public:
_SetupDiGetClassDevsW(const GUID& guid)
{
_list = SetupDiGetClassDevsW(&guid, NULL, NULL, DIGCF_PRESENT | DIGCF_INTERFACEDEVICE);
// Failure communicated by GetLastError() from SetupDiGetClassDevsW()
}
~_SetupDiGetClassDevsW()
{
if (_list != INVALID_HANDLE_VALUE) SetupDiDestroyDeviceInfoList(_list);
}
HDEVINFO list() { return _list; }
private:
HDEVINFO _list{ INVALID_HANDLE_VALUE };
};
class _SetupDiGetDeviceInterfaceDetailW
{
public:
_SetupDiGetDeviceInterfaceDetailW(HDEVINFO list, PSP_DEVICE_INTERFACE_DATA info)
{
DWORD size{ 0 };
BOOL success{ FALSE };
SetupDiGetDeviceInterfaceDetailW(list, info, NULL, 0, &size, NULL);
if (size == 0)
{
SetLastError(ERROR_INVALID_HANDLE);
return;
}
_details = (PSP_DEVICE_INTERFACE_DETAIL_DATA_W)calloc(size, 1);
if (_details == NULL)
{
SetLastError(ERROR_OUTOFMEMORY);
return;
}
_details->cbSize = sizeof(SP_DEVICE_INTERFACE_DETAIL_DATA_W);
success = SetupDiGetDeviceInterfaceDetailW(list, info, _details, size, &size, NULL);
if (success == FALSE)
{
SetLastError(ERROR_INVALID_HANDLE);
return;
}
}
~_SetupDiGetDeviceInterfaceDetailW()
{
if (_details != NULL) free(_details);
}
wchar_t* path() { return (_details) ? _details->DevicePath : NULL; }
private:
PSP_DEVICE_INTERFACE_DETAIL_DATA_W _details{ NULL };
};
int main()
{
BOOL success{ TRUE };
SP_DEVICE_INTERFACE_DATA info{ sizeof(SP_DEVICE_INTERFACE_DATA) };
HIDD_ATTRIBUTES attributes{ sizeof(HIDD_ATTRIBUTES) };
DWORD index{ 0 };
GUID guid{ 0 };
HidD_GetHidGuid(&guid);
_SetupDiGetClassDevsW hids(guid);
if (GetLastError()) return 0;
while (SetupDiEnumDeviceInterfaces(hids.list(), 0, &guid, index, &info))
{
index++;
_SetupDiGetDeviceInterfaceDetailW details(hids.list(), &info);
if (GetLastError()) continue;
_CreateFileW file(details.path());
if (GetLastError()) continue;
success = HidD_GetAttributes(file.handle(), &attributes);
if (!success) continue;
printf("Device attributes: VID=%d PID=%d Version=%4.4X\n",
attributes.VendorID, attributes.ProductID, attributes.VersionNumber);
}
return 0;
}
Defer
This is essentially the same as the RAII solution but using lambdas and putting the work into a set of macros, a class, and a function template. The function template is way to capture the lambda’s type which is actually internal to the compiler, so this is a bit of a templating trick.
The result is quite nice, in that, again, the code to construct and tear down are next to each other. There are a few subtleties to this defer trick, but if it is used in a simple way like this then it would be fine. The structure of the code needs to be kept across any future edits, it needs to three parts, create – test – defer tear down. The scope needs to be carefully checked as well, as the implicit/invisible variable will go out of scope as the end of the block and any future changes could change that.
Thus, hiding the variable so it can’t be used (as is the case in other more detailed implementations) is a good idea as this means there is less to go wrong. This also means there is no need for the copy and move operators. Finally, this is not being compiled with exceptions, so the destructor does not need the noexcept.
To keep the use simple is uses capture all by reference [&]. While it is clear this is not the right choice for complex cases this can be changed if they arise, also this style is not expected to be used in a performance critical code path.
#include <windows.h>
#include <hidsdi.h> // Link with: hib.lib
#include <setupapi.h> // Link with: setupapi.lib
#include <stdio.h>
template <typename lambdaType>
class deferClass
{
lambdaType _lambda;
public:
deferClass(lambdaType lambda) : _lambda(lambda) {}
~deferClass() { _lambda(); }
deferClass& operator=(const deferClass&) = delete;
deferClass(const deferClass&) = delete;
};
template <typename lambdaType>
deferClass<lambdaType> deferFunction(lambdaType lambda)
{
return deferClass<lambdaType>(lambda);
}
#define deferStringify(x, y) x##y
#define deferVariable(x, y) deferStringify(x, y)
#define defer(code) auto deferVariable(_defer, __LINE__) = deferFunction([&](){code;})
int main()
{
BOOL success{ TRUE };
SP_DEVICE_INTERFACE_DATA info{ sizeof(SP_DEVICE_INTERFACE_DATA) };
HIDD_ATTRIBUTES attributes{ sizeof(HIDD_ATTRIBUTES) };
DWORD index{ 0 };
GUID guid{ 0 };
DWORD size{ 0 };
PSP_DEVICE_INTERFACE_DETAIL_DATA_W details{ NULL };
HDEVINFO list{ NULL };
HANDLE handle{ INVALID_HANDLE_VALUE };
DWORD access{ GENERIC_READ | GENERIC_WRITE };
DWORD share{ FILE_SHARE_READ | FILE_SHARE_WRITE };
HidD_GetHidGuid(&guid);
list = SetupDiGetClassDevsW(&guid, NULL, NULL, DIGCF_PRESENT | DIGCF_INTERFACEDEVICE);
if (GetLastError()) return 0;
defer(SetupDiDestroyDeviceInfoList(list));
while (SetupDiEnumDeviceInterfaces(list, 0, &guid, index, &info))
{
index++;
size = 0;
SetupDiGetDeviceInterfaceDetailW(list, &info, NULL, 0, &size, NULL);
if (size == 0) continue;
details = (PSP_DEVICE_INTERFACE_DETAIL_DATA_W)calloc(size, 1);
if (details == NULL) continue;
defer(free(details));
details->cbSize = sizeof(SP_DEVICE_INTERFACE_DETAIL_DATA_W);
success = SetupDiGetDeviceInterfaceDetailW(list, &info, details, size, &size, NULL);
if (!success) continue;
handle = CreateFileW(details->DevicePath, access, share, NULL, OPEN_EXISTING, 0, NULL);
// If owned by OS or other processes this will fail; just ignore these.
if (handle == INVALID_HANDLE_VALUE) continue;
defer(CloseHandle(handle));
success = HidD_GetAttributes(handle, &attributes);
if (!success) continue;
printf("Device attributes: VID=%d PID=%d Version=%4.4X\n",
attributes.VendorID, attributes.ProductID, attributes.VersionNumber);
}
return 0;
}
There are lots of discussion on this approach and here are few links so as to make a review of those approaches easier.
- https://www.gingerbill.org/article/2015/08/19/defer-in-cpp/
- http://the-witness.net/news/2012/11/scopeexit-in-c11/
- https://dtrugman.medium.com/gos-defer-like-implementation-for-c-a91344bb08bf
- https://ricab.github.io/scope_guard/
- https://crascit.com/2015/05/27/on-leaving-scope-part-1/
- https://crascit.com/2015/06/03/on-leaving-scope-part-2/
- https://stackoverflow.com/questions/50182244/simple-way-to-execute-code-at-the-end-of-function-scope
Output Code in Godbolt
The line counts were found by compiling for space /O1, turning off buffer security /GS-, and then removing the excess lines (keeping just PROC to ENDP). The Jumps were found by adding up the instructions: call, je, jmp and jne.
| Style | ASM Lines | ASM Jumps | Godbolt Link |
|---|---|---|---|
| Indent | 114 | 22 | https://godbolt.org/z/nocbhfrfT |
| Goto | 114 | 22 | https://godbolt.org/z/seoj968jE |
| Never Nester | 114 | 22 | https://godbolt.org/z/d73hTzKbK |
| Never Nester Inner | 124 | 23 | https://godbolt.org/z/ehqojhxdc |
| RAII | 256 | 42 | https://godbolt.org/z/dj17TTx44 |
| Defer | 1141 | 21 | https://godbolt.org/z/s6v9ejnhc |
1 The output for the Defer Style appears to include code that is never run. So, this count is higher than it should be. But if you link the code that excess code is removed and it gets back to the optimal assembly.
Conclusion
So, do any of them stand out? It seems that Goto is a good option regardless of the negative press it gets. If the code management and testing could be managed, then it still seems like a viable option. While I personally like the Never Nester style, having to do added functions to fix the repeated teardowns means it should be taken as a general approach when possible and not used all the time.
I liked the RAII solution in using the scope and it seems to generate the cleanest mainline code. However, in trying the Defer method I really like it. Defer is in all the new languages (even proposed for future versions of C++) and it does seem like the right way to think about these sorts of calls and resources. Deal with it there and then, without having to break out and create a set of new classes.
It will be interesting to see what I think of all this a few years into my journey of learning C++.
Mark likes to have a number of side projects on the go. Currently learning C++ and Vulkan. Other areas of interest include photography and going for walks.