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

Revert "fix : including windows.h header cause error" #263

Merged
merged 1 commit into from Aug 5, 2019
Merged

Conversation

onqtam
Copy link
Member

@onqtam onqtam commented Aug 5, 2019

@onqtam onqtam merged commit d70ff74 into dev Aug 5, 2019
@onqtam onqtam deleted the revert-258-dev branch August 7, 2019 07:04
@ghost
Copy link

ghost commented Nov 21, 2019

Can you please state the rationale behind this revert? Our clang builds on Windows fail with -Wnonportable-system-include-path because of this. Thank you.

@onqtam
Copy link
Member Author

onqtam commented Nov 21, 2019

@suoniq this revert is not for the windows.h include but for the addition of <double> in the call to std::max - look at the changes in the PR. The original PR was broken because it was missing a paren (as can be seen in the changes).

Currently windows.h is included like this: #include <Windows.h> - does version 2.3.5 not work for you? If not - that could easily be changed.

@ghost
Copy link

ghost commented Nov 21, 2019

this revert is not for the windows.h include

Then I got distracted by the issue description. Sorry.

Currently windows.h is included like this: #include <Windows.h> - does version 2.3.5 not work for you? If not - that could easily be changed.

Exactly. It has to be #include <windows.h> (small caps) to silence Clang using [-Werror,-Wnonportable-system-include-path]. Then it works on Windows/Linux X gcc/clang - each with very picky warning flags. I located three ill-formed includes. Will open a MR fixing that.

@onqtam onqtam changed the title Revert "fix : includeing windows.h header caause error" Revert "fix : including windows.h header cause error" Nov 5, 2021
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

1 participant