oneTBB
oneTBB copied to clipboard
Move enumerable_thread_specific Windows.h usage to cpp
Description
This change gets rid of the Windows.h include in the public enumerable_thread_specific .h
by moving it a cpp.
Fixes #573
Type of change
- [x] bug fix - change which fixes an issue
- [ ] new feature - change which adds functionality
- [ ] tests - change in tests
- [ ] infrastructure - change in infrastructure and CI
- [ ] documentation - documentation update
Tests
- [ ] added - required for new features and for some bug fixes
- [x] not needed
Documentation
- [ ] updated in # - add PR number
- [ ] needs to be updated
- [x] not needed
Breaks backward compatibility
- [ ] Yes
- [x] No
- [ ] Unknown
Can it affect performance?
The Tls*
and Fls*
functions are themselves dllimport, so I wouldn't expect this change to have a significant impact.
Before answering above questions, can we consider the option to use processthreadsapi.h (or fibersapi.h with __TBB_WIN8UI_SUPPORT)?
I'm not sure whether you're supposed to include processthreadsapi.h
without windows.h
. If we want to do that, we would have to replicate the architecture detection block from windows.h
because processthreadsapi.h
needs one of these symbols to be defined.
If you would prefer that, I can also create a PR for using processthreadsapi.h
. But I do feel a bit uneasy about copying that architecture code.
@Chronial is there any estimates or real measurements of build times reduction effect of this patch ?
This change won't have a massive impact on our build times. In places where this is a serious issue, enumerable_thread_specific.h
is simply added to the precompiled headers.
This is more about usability:
-
Windows.h
includes are affected by various macros, so it should be included before some and after some other headers. This now affectsenumerable_thread_specific.h
, which makes it more error-prone to use. -
Windows.h
defines countless names as macros, likeSetEvent
,SetWindowText
,RegisterClass
and many more. This makes these identifiers unusable in any file that includes Windows.h. This doesn't just affect global names, but also as method names and names in any namespace. This property is now inherited byenumerable_thread_specific
. -
Windows.h
notoriously defines the namesmin
andmax
as macros, breakingstd::min
andstd::max
with nice error messages. - Due to the points above and the size of Windows.h, you are strongly discouraged from including
enumerable_thread_specific
in any of our headers. So enumerable_thread_specific has to be hidden with pimpl or simply can't be used in templates.
@alexey-katranov Is there anything I can do to move this forward?
ping :)
@pavelkumbrasev , I believe now it's your chance to review it :)