Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Windows: stop including <windows.h> from public OIIO headers #3597

Merged
merged 1 commit into from
Oct 12, 2022
Merged

Windows: stop including <windows.h> from public OIIO headers #3597

merged 1 commit into from
Oct 12, 2022

Conversation

aras-p
Copy link
Contributor

@aras-p aras-p commented Oct 11, 2022

Description

Including windows.h leads to a lot of possibly common names being "taken", mostly by preprocessor defines. Some of these are possible to turn off (like "max" and "min" by defining "NOMINMAX" before including <windows.h>). But many others are not possible to turn off, e.g. "near" and "far" just get defined to nothing, preventing from using variables named like that.

Additionally, includes have a cost, and while <windows.h>, being a C header does not have expensive language constructs that would slow down compilation a lot, it's still something close to 100k lines of preprocessed code to wade through.

Now, only the OIIO implementation .cpp files that actually need <windows.h> include it (all of them under "libutil").

There were two exceptions:

  1. thread.h pause() implementation uses YieldProcessor() macro when on
    a Windows system. Replaced it with manually "re-implemented"
    things that YieldProcessor expands to. Right now made the
    implementation for all 4 platforms that Windows normally supports
    (x64, x86, arm, arm64); the latter two OIIO does not support though
    when on Windows so it's untested.
  2. timer.h Timer::now() inline function used QueryPerformanceCounter
    and LARGE_INTEGER. These are quite hard to somehow "forward-declare"
    or similar, without resulting in duplicate type definitions. So on
    Windows I moved the now() function to be non-inline. A though for another
    day: maybe all of that could be replaced by C++ <chrono> instead.

Tests

Existing CI tests.

Checklist:

  • I have read the contribution guidelines.
  • If this is more extensive than a small change to existing code, I
    have previously submitted a Contributor License Agreement
    (individual, and if there is any way my
    employers might think my programming belongs to them, then also
    corporate).
  • I have updated the documentation, if applicable.
  • I have ensured that the change is tested somewhere in the testsuite
    (adding new test cases if necessary).
  • My code follows the prevailing code style of this project.

@lgritz
Copy link
Collaborator

lgritz commented Oct 11, 2022

Yeah, the timer.h predates being able to depend on C++11 chrono. We'll want to benchmark to be sure that on all the platforms, chono is not higher overhead than when we're currently doing, but if it's good, I would have no objection to a future timer.h cleanup that bases it on chrono wherever possible.

src/libutil/filesystem.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@lgritz lgritz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I like getting windows.h out of the public headers.

Including windows.h leads to a lot of possibly common names being
"taken", mostly by preprocessor defines. Some of these are possible
to turn off (like "max" and "min" by defining "NOMINMAX" before
including <windows.h>). But many others are not possible to turn off,
e.g. "near" and "far" just get defined to nothing, preventing from using
variables named like that.

Additionally, includes have a cost, and while <windows.h>, being a C
header does not have expensive language constructs that would slow down
compilation a lot, it's still something close to 100k lines of
preprocessed code to wade through.

Now, only the OIIO implementation .cpp files that actually need
<windows.h> include it (all of them under "libutil").

There were two exceptions:
1) thread.h pause() implementation uses YieldProcessor() macro when on
  a Windows system. Replaced it with manually "re-implemented"
  things that YieldProcessor expands to. Right now made the
  implementation for all 4 platforms that Windows normally supports
  (x64, x86, arm, arm64); the latter two OIIO does not support though
  when on Windows so it's untested.
2) timer.h Timer::now() inline function used QueryPerformanceCounter
  and LARGE_INTEGER. These are quite hard to somehow "forward-declare"
  or similar, without resulting in duplicate type definitions. So on
  Windows I moved the now() function to be non-inline.
@lgritz lgritz merged commit c721423 into AcademySoftwareFoundation:master Oct 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants