oneTBB
oneTBB copied to clipboard
oneapi-src
Remove <Windows.h> include from headers
The tbb currently includes Windows.h in enumerable_thread_specific.h. Through transitive includes, this also affects combinable.h, concurrent_map.h and concurrent_set.h.
Since windows.h is massive and declares thousands of symbols, this include has a negative impact on build times. The windows include does not need to be in a public header. We should either:
- Copy the declarations of the few required symbols from windows.h or
- Move the calls to the Win32 API to a cpp
to notice :
detail/_machine.h includes windows.h as well, however only for TBB Malloc build (#ifdef __TBBMALLOC_BUILD).
Two thumbs up from my side for reducing the use of windows.h in headers...
Please remove windows.h from shared header files for tests in test/common. Its presence has created difficulties for the cygwin port that needs Win32 versions of certain functions while mostly defaulting to Linux and Unix code. A particular example is GetMemoryUsage in test/common/memory_usage.h as described in https://github.com/oneapi-src/oneTBB/issues/156#issuecomment-963280611
@Chronial notes in their PR https://github.com/oneapi-src/oneTBB/pull/576#issuecomment-957730990 that this issue is about usability
I'll point out that oneTBB is a portable library that provides cross-platform abstractions and interfaces. C++ headers basically define these interfaces. Including windows.h in a header file unfortunately violates these portable abstractions in the worst possible way - by polluting the global namespace with macros that have nothing to do with oneTBB interfaces.
Also it's not like oneTBB is a header-only library that does not separate implementation from interface - this request is just along the same lines as what oneTBB has been doing all along. It's simply that one needs to think of windows.h as an implementation detail that oneTBB needs to encapsulate within its abstraction layer - any other considerations are secondary
oneTBB is a mix of header-only and binary-based APIs; many class templates are header-only at least by default (if no tool support is requested via a macro, etc.). Typically these templates, including enumerable_thread_specific and concurrent containers, can be used not only in oneTBB parallel constructs but in any thread. IIRC in the past there were requests to allow using such APIs without linking with TBB binaries. which is also about usability.
Therefore, even though the use of windows.h is an implementation detail, whether to include it in public headers or not is an important tradeoff that needs to be carefully evaluated. This is what the team is doing now.
One possible solution I can think of is to provide an opt-out macro that precludes the use of windows.h at the cost of making ets_key_per_instance storage type unavailable (as simply changing the behavior of that would create risk of ODR violation).
Probably NOMINMAX can decrease the overhead of Windows.h inclusion, while this is the solution for a different issue.
Probably
NOMINMAXcan decrease the overhead of Windows.h inclusion, while this is the solution for a different issue.
This preprocessor define is certainly helpful. In case someone has this problem, there are even more related macros, see http://web.archive.org/web/20121219084749/http://support.microsoft.com/kb/166474:
VC_EXTRALEAN
WIN32_LEAN_AND_MEAN
However, this does not really solve all the problems that can be caused by including windows.h. For example, the MSVC header will still add defines for a number of common names. For example, a number of file API methods are using preprocessor defines: CreateFile, DeleteFile, etc from https://learn.microsoft.com/en-us/windows/win32/api/fileapi/. After including windows.h, these names can no longer be used as class or method names.
In summary, its always very good to avoid including windows.h in any headers, if somehow possible. No problem to include it in implementation files, though.
