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

VS2015/2017 build fix and enable C++11 features #811

Closed
wants to merge 3 commits into from

Conversation

KindDragon
Copy link
Contributor

@KindDragon KindDragon commented Jun 27, 2016

Depend on PR #723

Commit 1695708 allow use C++11 features available in VS2015, fix failed tests with GMock under VC++2015 compiler

@@ -71,7 +71,7 @@
TypedTest/0\. # TypeParam = (VeryLo{245}|class VeryLo{239})\.\.\.
TestA
TestB
TypedTest/1\. # TypeParam = int\s*\*
TypedTest/1\. # TypeParam = int\s*\*( __ptr64)?
Copy link
Contributor

Choose a reason for hiding this comment

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

There seems to be a lot going on here that isn't technically necessary.
I would like to have some idea of what's being changed but I can't review all of this at once.
Can you write a little in the "conversation" tab about what exactly is being changed and why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This commit disappear after you merge PR #723. So ignore this commit here.
Specifically, these changes correct VC++ 64-bit tests

@coreykosak
Copy link

I'm curious about something. We used to rely on CMake to auto-generate our Visual Studio solution and project files. Are we no longer doing that?

@@ -355,7 +355,9 @@
#if GTEST_STDLIB_CXX11
# define GTEST_HAS_STD_BEGIN_AND_END_ 1
# define GTEST_HAS_STD_FORWARD_LIST_ 1
# define GTEST_HAS_STD_FUNCTION_ 1
# if !defined(_MSC_VER) || (_MSC_FULL_VER >= 190023824) // works only with VS2015U2 and better
# define GTEST_HAS_STD_FUNCTION_ 1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

VS2015 without updates doesn't support all required std::function features

@KindDragon
Copy link
Contributor Author

@coreykosak I only want support old projects for old VS (2010 and less), because some users depend on them. Old VS projects cannot run tests. New users should use CMake generated projects.

But now we have VS2015 projects #773 😞

@coreykosak
Copy link

I guess I don't understand the full story. CMake should be able to generate projects for any Visual Studio version, and it can support conditionals to enable or disable tests on some versions.

This is my assumption anyway. I haven't actually tried it.

@KindDragon
Copy link
Contributor Author

Yes you right, but some people prefer don't use CMake and they use old projects.
We can remove old projects at some point.

@coreykosak
Copy link

I agree. Sorry, I didn't say my whole thought, which is.

Even if we publish the solution and project files for all VS versions (and I think we should), those files should be generated by CMake, not hand-edited. Put another way, our job is to get CMakeLists.txt right, run it for all the versions, and then publish the result.

@KindDragon
Copy link
Contributor Author

@BillyDonahue anything else is not clear in this PR?

@KindDragon
Copy link
Contributor Author

@BillyDonahue any news? I promise that when we will fix VS 2015 build I stop posting messages there 😄

@@ -515,7 +515,7 @@ template <typename T, typename M>
class MatcherCastImpl {
public:
static Matcher<T> Cast(const M& polymorphic_matcher_or_value) {
// M can be a polymorhic matcher, in which case we want to use
// M can be a polymorphic matcher, in which case we want to use
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not an MSVC build fix.
Please narrow your changes down for review.

If you want to fix spelling errors, do it separately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

@KindDragon
Copy link
Contributor Author

Rebased to remove changes which were in PR #821

@KindDragon
Copy link
Contributor Author

@BillyDonahue Do I need remove anything else from PR?

// Determines if unordered_map/unordered_set are available.
// Only used for testing against those containers.
#if !defined(GTEST_HAS_UNORDERED_MAP_)
# if defined(_MSC_VER) && (_MSC_VER >= 1900)
Copy link

Choose a reason for hiding this comment

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

std::unordered_set and std::unordered_map tests should also be enabled on other platforms, I would say this check should be #if GTEST_LANG_CXX11

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Somebody can try do that in separate PR. I am trying to merge this changes since 23 February 😄

@KindDragon
Copy link
Contributor Author

@BillyDonahue PR now has only one commit. VS2010 solution changes moved to separate PR.

@KindDragon KindDragon force-pushed the vs-build-fix branch 2 times, most recently from ba08175 to 1695708 Compare August 15, 2016 13:56
@marco-m
Copy link
Contributor

marco-m commented Sep 22, 2016

I agree with @coreykosak: cmake should be used to detect what the compiler supports, instead of adding conditional macro logic in gtest include files.

If somebody is using _old_version of VS and doesn't want to run cmake, why don't they simply use an old version of gtest? :-)

@KindDragon
Copy link
Contributor Author

KindDragon commented Sep 22, 2016

I don't add it. I just update currently used #ifdef`s

If you know how to do that, patches welcome.

@KindDragon
Copy link
Contributor Author

I agree with @coreykosak: cmake should be used to detect what the compiler supports, instead of adding conditional macro logic in gtest include files.

Why do we have to do it, if condional macro works pretty fine for all use cases?

@KindDragon KindDragon mentioned this pull request Oct 15, 2016
@KindDragon KindDragon mentioned this pull request Mar 18, 2017
@KindDragon KindDragon force-pushed the vs-build-fix branch 4 times, most recently from 6c64f2a to c3685ac Compare May 3, 2017 11:37
@KindDragon KindDragon changed the title VS2015 build fix and enable C++11 features VS2015/2017 build fix and enable C++11 features May 3, 2017
BhaaLseN pushed a commit to BhaaLseN/dolphin that referenced this pull request May 26, 2017
BhaaLseN pushed a commit to BhaaLseN/dolphin that referenced this pull request May 28, 2017
spycrab pushed a commit to spycrab/dolphin that referenced this pull request Jun 3, 2017
Neui pushed a commit to Neui/dolphin-emu that referenced this pull request Jun 6, 2017
@gennadiycivil
Copy link
Contributor

This looks somewhat messy, a lot of conversation, merge conflicts now.
I am closing it and looking forward to clean PR that can be discussed anew and actually merged in.

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

7 participants