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.

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

Fix for Windows compiler, missing object initializer #4188

Merged
merged 2 commits into from
Sep 20, 2022
Merged

Fix for Windows compiler, missing object initializer #4188

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4c01d47
Fix for windows compiler, missing object initializer
jiwaszki Sep 19, 2022
cef7a44
Removal of if-else macro for MSVC
jiwaszki Sep 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion include/pybind11/detail/common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1037,7 +1037,7 @@ template <typename... Args>
|| (defined(__clang__) && __clang_major__ == 5)
static constexpr detail::overload_cast_impl<Args...> overload_cast = {};
# else
static constexpr detail::overload_cast_impl<Args...> overload_cast;
static constexpr detail::overload_cast_impl<Args...> overload_cast{};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Uhm ... wait, do we actually need this ugly #ifdef?

Copy link
Collaborator

Choose a reason for hiding this comment

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

You mean maybe the new version works on all compilers?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also why weren't we seeing the errors with newer MSVC? We should be testing with 2019 & 2022 (maybe not 2017 anywhere anymore).

Copy link
Collaborator

Choose a reason for hiding this comment

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

You mean maybe the new version works on all compilers?

Yes, I tried it out, CI is green: #4190

Whatever made the #ifdef necessary in the past, it doesn't seem to be an issue anymore with the compilers we support today.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uhm ... wait, do we actually need this ugly #ifdef?

Actually if the {} is present, this #ifdef can be removed. I will cover that in next commit and run checks.

Also why weren't we seeing the errors with newer MSVC?

My bad, I have got lost in the logs of the project and actual MSVC build version is 1924. Maybe this version is not covered? Also I see that -DCMAKE_CXX_STANDARD=17/20 is set for Windows builds, is C++14 covered as well?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I bet no one ever tried it with {}, but only with = {}.

I'm hoping to get a patch release out in ~1 week or so. Still have a few items on #4125 left.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@henryiii Thanks for the answer, can't wait to finally bump version in OpenVINO!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@henryiii As fix version 2.10.1 is going to be released, will it include this PR and everything from the master branch up to this point? I am not accustom to pybind's release practices, I am really sorry for such questions.

(I do not want to spam in #4125 comments, hope you don't mind:))

Copy link
Collaborator

@henryiii henryiii Oct 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

It should. I think there might be one or two breaking changes which could go in before it releases, which means we'll branch from there, but so far master is 2.10.1. I'm going to finish investigation into the CMake caching issue in the next couple of days, then make a release.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Missed my target, but 2.10.1 is out now.

# endif
#endif

Expand Down