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

VS2015/2017 build fix and enable C++11 features (attempt number three) #1218

Merged
merged 3 commits into from
Oct 25, 2017
Merged

VS2015/2017 build fix and enable C++11 features (attempt number three) #1218

merged 3 commits into from
Oct 25, 2017

Conversation

KindDragon
Copy link
Contributor

@KindDragon KindDragon commented Aug 21, 2017 •
edited
Loading

Allow use C++11 features available in VS2015/2017, fix failed tests with GMock under VC++2015 compiler
This is rebased PR #811

I have been trying to merge this change since February 2016 (PR #723)

@KindDragon KindDragon changed the title VS2015/2017 build fix and enable C++11 features (attempt number two) VS2015/2017 build fix and enable C++11 features (attempt number three) Aug 21, 2017
@KindDragon KindDragon mentioned this pull request Aug 24, 2017
Merged
@pavelkryukov pavelkryukov mentioned this pull request Sep 23, 2017
Merged
@KindDragon KindDragon mentioned this pull request Oct 3, 2017
Closed
@coder0xff
Copy link

My build process has been cloning from your branches for months. Thanks for making gtest work in VS.

@pavelkryukov pavelkryukov mentioned this pull request Oct 18, 2017
Merged
@KindDragon
Copy link
Contributor Author

@gennadiycivil Can we move forward with this PR?

@KindDragon KindDragon mentioned this pull request Oct 24, 2017
Closed
@gennadiycivil
Copy link
Contributor

I started a review but did not get any answers...

@KindDragon
Copy link
Contributor Author

KindDragon commented Oct 24, 2017 •
edited
Loading

Did you send questions to me? Perhaps you forgot to click the Finish your review button (happened to me several times)

gennadiycivil
gennadiycivil reviewed Oct 24, 2017
View reviewed changes
googlemock/test/gmock-generated-matchers_test.cc
@@ -120,21 +120,21 @@ TEST(ArgsTest, AcceptsOneTemplateArg) {
}

TEST(ArgsTest, AcceptsTwoTemplateArgs) {
const tuple<short, int, long> t(4, 5, 6L); // NOLINT
const tuple<short, int, long> t(static_cast<short>(4), 5, 6L); // NOLINT
Copy link
Contributor

Choose a reason for hiding this comment

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

Why? Please explain static_cast and exactly why its needed here

Copy link
Contributor

Choose a reason for hiding this comment

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

@KindDragon

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 line produces warning tuple(165): warning C4244: 'initializing': conversion from 'int' to 'short', possible loss of data and because of -WX compiler produce errors for warnings

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same bellow

Copy link

Choose a reason for hiding this comment

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

The same change was proposed in #1136

googlemock/test/gmock-generated-matchers_test.cc

EXPECT_THAT(t, (Args<0, 1>(Lt())));
EXPECT_THAT(t, (Args<1, 2>(Lt())));
EXPECT_THAT(t, Not(Args<0, 2>(Gt())));
}

TEST(ArgsTest, AcceptsRepeatedTemplateArgs) {
const tuple<short, int, long> t(4, 5, 6L); // NOLINT
const tuple<short, int, long> t(static_cast<short>(4), 5, 6L); // NOLINT
Copy link
Contributor

Choose a reason for hiding this comment

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

Why? Please explain static_cast and exactly why its needed here

Copy link
Contributor

Choose a reason for hiding this comment

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

@KindDragon

googlemock/test/gmock-generated-matchers_test.cc
EXPECT_THAT(t, (Args<0, 0>(Eq())));
EXPECT_THAT(t, Not(Args<1, 1>(Ne())));
}

TEST(ArgsTest, AcceptsDecreasingTemplateArgs) {
const tuple<short, int, long> t(4, 5, 6L); // NOLINT
const tuple<short, int, long> t(static_cast<short>(4), 5, 6L); // NOLINT
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto

Copy link
Contributor

Choose a reason for hiding this comment

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

@KindDragon

googlemock/test/gmock-generated-matchers_test.cc
@@ -159,7 +159,7 @@ TEST(ArgsTest, AcceptsMoreTemplateArgsThanArityOfOriginalTuple) {
}

TEST(ArgsTest, CanBeNested) {
const tuple<short, int, long, int> t(4, 5, 6L, 6); // NOLINT
const tuple<short, int, long, int> t(static_cast<short>(4), 5, 6L, 6); // NOLINT
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto

googlemock/test/gmock-matchers_test.cc
@@ -58,6 +58,11 @@
# include <forward_list> // NOLINT
#endif

// Disable MSVC2015 warning for std::pair: "decorated name length exceeded, name was truncated".
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it really only apply to 2015? What happens in 2017? I would want to see test results for both

Copy link
Contributor

Choose a reason for hiding this comment

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

@KindDragon

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think these changes fix compiler warning:
9c1bb54

I'll check that

Copy link
Contributor Author

@KindDragon KindDragon Oct 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

VS 2015 produce this error:

     7>D:\Work\googletest\googlemock\test\gmock-matchers_test.cc(5683): error C2220: warning treated as error - no 'object' file generated [D:\Work\goo
       gletest\build\Win32\googlemock\gmock-matchers_test.vcxproj]
     7>D:\Work\googletest\googlemock\test\gmock-matchers_test.cc(5683): warning C4503: 'std::pair<Head,std::pair<Head,std::pair<Head,std::pair<Head,std
       ::pair<Head,std::pair<Head,std::pair<Head,std::pair<Head,std::pair<Head,std::pair<Head,std::pair<Head,std::pair<Head,std::pair<Head,std::pair<He
       ad,std::pair<Head,std::pair<Head,std::pair<Head,std::pair<Head,std::pair<Head,std::pair<Head,std::pair<Head,std::pair<Head,std::pair<Head,std::p
       air<Head,std::pair<Head,std::pair<Head,std::pair<Head,std::pair<Head,std::pair<Head,std::pair<Head,std::pair<Head,std::pair<Head,std::pair<Head,
       std::pair<Head,std::pair<Head,std::pair<Head,std::pair<Head,std::pair<Head,std::pair<Head,std::pair<Head,std::pair<Head,std::pair<Head,std::pair
       <Head,std::pair<Head,std::pair<Head,std::pair<Head,std::pair<Head,std::pair<Head,std::pair<Matcher1,Matcher2>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
       >>>>>>>>>>>>>::pair': decorated name length exceeded, name was truncated [D:\Work\googletest\build\Win32\googlemock\gmock-matchers_test.vcxproj]
                 with
                 [
                     Head=testing::internal::NeMatcher<int>,
                     Matcher1=testing::internal::NeMatcher<int>,
                     Matcher2=testing::internal::NeMatcher<int>
                 ]

VS2017 compile gtest without warnings (maybe because of variadic templates)

@gennadiycivil
Copy link
Contributor

Oh yes, thank you this is exactly what I did(not) do

@gennadiycivil
Copy link
Contributor

AppVeyor build failed - here

@KindDragon
Copy link
Contributor Author

KindDragon commented Oct 25, 2017 •
edited
Loading

@gennadiycivil This should be now fixed

@gennadiycivil gennadiycivil merged commit 7684db3 into google:master Oct 25, 2017
@KindDragon KindDragon deleted the vs-build-fix branch October 25, 2017 15:28
@KindDragon
Copy link
Contributor Author

Hooray 🎉🎉🎉

This was referenced Oct 25, 2017
Closed
Closed
Closed
This was referenced Oct 25, 2017
Closed
Closed
Closed
Closed
Closed
Closed
bilke pushed a commit to bilke/ogs that referenced this pull request Dec 19, 2018
@endJunction @bilke
Add current googletest version as submodule.
63f7e10 
The original gtest folder is removed.

Reason is the removed namespace std::tr1 no longer available in the VS2017.
Fixed in PR google/googletest#1218
and the related issue is google/googletest#1138
jbathmann pushed a commit to jbathmann/ogs that referenced this pull request Mar 5, 2019
@endJunction
Add current googletest version as submodule.
799a74c 
The original gtest folder is removed.

Reason is the removed namespace std::tr1 no longer available in the VS2017.
Fixed in PR google/googletest#1218
and the related issue is google/googletest#1138
wolf-pf pushed a commit to wolf-pf/ogs that referenced this pull request Mar 27, 2019
@endJunction @wolf-pf
Add current googletest version as submodule.
2f0a50c 
The original gtest folder is removed.

Reason is the removed namespace std::tr1 no longer available in the VS2017.
Fixed in PR google/googletest#1218
and the related issue is google/googletest#1138
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gennadiycivil gennadiycivil gennadiycivil left review comments

@pavelkryukov pavelkryukov pavelkryukov left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@KindDragon @coder0xff @gennadiycivil @pavelkryukov