Skip to content

Add NOMINMAX define due to windows.h max/min macro conflict #25637

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 23, 2020
Merged

Add NOMINMAX define due to windows.h max/min macro conflict #25637

merged 1 commit into from
Jul 23, 2020

Conversation

@wozna
Copy link
Contributor

@wozna wozna commented Jul 21, 2020 •
edited
Loading

PR types

Bug fixes

PR changes

Others

Describe

This PR adds NOMINMAX define before each windows.h. It is due to problem with macro min/max that conflict with std::min/max.

@paddle-bot-old
Copy link

paddle-bot-old bot commented Jul 21, 2020

Thanks for your contribution!
Please wait for the result of CI firstly. See Paddle CI Manual for details.

@paddle-bot-old
Copy link

paddle-bot-old bot commented Jul 21, 2020

❌This PR is not created using PR's template. You can refer to this Demo.
Please use PR's template, it helps save our maintainers' time so that more developers get helped.

@wozna wozna added Intel windows Windows related issues labels Jul 21, 2020
@wozna wozna requested review from grygielski and jczaja July 21, 2020 07:56
jczaja
jczaja approved these changes Jul 21, 2020
View reviewed changes
Copy link
Contributor

@jczaja jczaja left a comment

Choose a reason for hiding this comment

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

LGTM

grygielski
grygielski approved these changes Jul 21, 2020
View reviewed changes
Copy link
Contributor

@grygielski grygielski left a comment

Choose a reason for hiding this comment

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

LGTM

@wozna
Copy link
Contributor Author

wozna commented Jul 23, 2020

@luotao1 Can I ask you for restarting PR_Windows_CI_OPENBLAS? It looks that it was a random fail connected to Lib.exe access.

@luotao1
Copy link
Contributor

luotao1 commented Jul 23, 2020 •
edited
Loading

I have restarted, please have a wait.

@luotao1 luotao1 requested a review from zhwesky2010 July 23, 2020 08:24
zhwesky2010
zhwesky2010 reviewed Jul 23, 2020
View reviewed changes
Copy link
Contributor

@zhwesky2010 zhwesky2010 left a comment

Choose a reason for hiding this comment

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

is there a bug already?, or just prevent future risks when add std::min/max in code

@wozna
Copy link
Contributor Author

wozna commented Jul 23, 2020

@zhouwei25 there was a problem adding a new data type bfloat16 #25402, so I decided to put it in a separate PR

@luotao1
Copy link
Contributor

luotao1 commented Jul 23, 2020

there was a problem adding a new data type bfloat16

Could you explain the detail of your problem?

zhwesky2010
zhwesky2010 approved these changes Jul 23, 2020
View reviewed changes
Copy link
Contributor

@zhwesky2010 zhwesky2010 left a comment

Choose a reason for hiding this comment

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

I get it. This problem is triggered when different header files are included together. It will prevent possible risks. LGTM

@luotao1 luotao1 merged commit e5bbffa into PaddlePaddle:develop Jul 23, 2020
@wozna wozna deleted the nominmax branch February 24, 2023 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zhwesky2010 zhwesky2010 zhwesky2010 approved these changes

+2 more reviewers

@jczaja jczaja jczaja approved these changes

@grygielski grygielski grygielski approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Intel windows Windows related issues

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@wozna @luotao1 @jczaja @grygielski @zhwesky2010