Fix all -Wdeprecated warnings; eliminate GTEST_DISALLOW_ASSIGN_ #2815

Quuxplusone · 2020-04-16T23:35:14Z

This is the spiritual successor to #2774 (which I tried and failed to reopen) and #2758.
Fixes #1305.

Remove all uses of GTEST_DISALLOW_{MOVE_,}ASSIGN_.

None of these are strictly needed for correctness.
A large number of them (maybe all of them?) trigger `-Wdeprecated`
warnings on Clang trunk as soon as you try to use the implicitly
defaulted (but deprecated) copy constructor of a class that has
deleted its copy assignment operator.

By declaring a deleted copy assignment operator, the old code
also caused the move constructor and move assignment operator
to be non-declared. This means that the old code never got move
semantics -- "move-construction" would simply call the defaulted
(but deprecated) copy constructor instead. With the new code,
"move-construction" calls the defaulted move constructor, which
I believe is what we want to happen. So this is a runtime
performance optimization.

Add -Wdeprecated to the build configuration.

The change in VariadicMatcher looks deceptively simple, but I actually still don't really understand what's going on with it; feel free to dig deeper if you can.

@mbxx @zhangxy988 please take a look!

…me performance. We are about to remove all uses of GTEST_DISALLOW_ASSIGN_ in favor of using the Rule of Zero everywhere. Unfortunately, if we use the Rule of Zero here, then when the compiler needs to figure out if VariadicMatcher is move-constructible, it will recurse down into `tuple<Args...>`, which on libstdc++ recurses too deeply. In file included from googlemock/test/gmock-matchers_test.cc:43: In file included from googlemock/include/gmock/gmock-matchers.h:258: In file included from /usr/include/c++/5.5.0/algorithm:60: In file included from /usr/include/c++/5.5.0/utility:70: In file included from /usr/include/c++/5.5.0/bits/stl_pair.h:59: In file included from /usr/include/c++/5.5.0/bits/move.h:57: /usr/bin/include/c++/5.5.0/type_traits:115:26: fatal error: recursive template instantiation exceeded maximum depth of 256 : public conditional<_B1::value, _B1, _B2>::type ^ The move constructor is the only problematic case, for some unknown reason. With GTEST_DISALLOW_ASSIGN_, the presence of a copy assignment operator causes the move constructor to be non-declared, thus non-defaulted, thus non-problematic. Without GTEST_DISALLOW_ASSIGN_, we have to do one of the following: - Default the copy constructor, so that the move constructor will be non-declared. - Define our own non-defaulted move constructor. ...except that doing the latter STILL did not work! Fortunately, the former (default the copy constructor, don't provide any move constructor) both works in practice and is semantically equivalent to the old code.

zhangxy988

Thanks for your efforts! We agree that these types should still be move constructible, however, we think it is better to explicitly spell it out, i.e. Type(Type&&) = default;, rather than relying on the presence of const members. Could you please add user-declared move ctors? Thanks again!

Quuxplusone · 2020-04-21T18:44:26Z

@zhangxy988: Sure, I think I'm amenable to =default'ed members. But can you show what you mean via a comment on a specific line of code, because I can't tell what exactly you mean by "these types." All types? Just the Matchers? Other? Does that include making changes to 0d2269c b543eb7 40827d9 as well?

Also, for types with const members, their move-constructor will actually copy-construct the const member (that's one reason I personally say "member data should never be const-qualified," which I guess I forgot to mention in my blog post on const). Godbolt. So when you say "these types should still be move constructible," do you mean that they should be cheaply move-constructible, i.e., I should remove the const from data members? (I expect you don't mean that, but if you did, I'd like to do it in yet another separate PR.)

zhangxy988

In general, I am OK with just mechanically replace GTEST_DISALLOW_ASSIGN_ with Type& operator=(const Type&) = delete;.
Move ctor can be considered separately.

googlemock/include/gmock/gmock-actions.h

googletest/test/googletest-port-test.cc

googlemock/include/gmock/gmock-actions.h

googlemock/include/gmock/gmock-spec-builders.h

googletest/test/googletest-param-test-test.cc

zhangxy988

After more thought, I think I agree with your approach here, i.e. rule of zero, I will import this for an internal review.

zhangxy988 · 2020-04-24T02:15:09Z

Though this macro is supposed to be internal, it's unfortunately used inside Google and removing it would cause breakages. I am going to clean them up, but you can choose to wait for my cleanup to complete (which delays this PR) or keep the macro for now. Let me know how you want to proceed.

None of these are strictly needed for correctness. A large number of them (maybe all of them?) trigger `-Wdeprecated` warnings on Clang trunk as soon as you try to use the implicitly defaulted (but deprecated) copy constructor of a class that has deleted its copy assignment operator. By declaring a deleted copy assignment operator, the old code also caused the move constructor and move assignment operator to be non-declared. This means that the old code never got move semantics -- "move-construction" would simply call the defaulted (but deprecated) copy constructor instead. With the new code, "move-construction" calls the defaulted move constructor, which I believe is what we want to happen. So this is a runtime performance optimization. Unfortunately we can't yet physically remove the definitions of these macros from gtest-port.h, because they are being used by other code internally at Google (according to zhangxy988). But no new uses should be added going forward.

gmock-spec-builders.h:503:3: error: definition of implicit copy constructor for 'Expectation' is deprecated because it has a user-declared destructor [-Werror,-Wdeprecated] ~Expectation(); ^

googletest-port-test.cc:97:11: error: definition of implicit copy constructor for 'Base' is deprecated because it has a user-declared destructor [-Werror,-Wdeprecated] virtual ~Base() {} ^

googletest-param-test-test.cc:502:8: error: definition of implicit copy constructor for 'NonDefaultConstructAssignString' is deprecated because it has a user-declared copy assignment operator [-Werror,-Wdeprecated] void operator=(const NonDefaultConstructAssignString&); ^

Quuxplusone · 2020-04-24T02:23:44Z

@zhangxy988 wrote:

Though this macro is supposed to be internal, it's unfortunately used inside Google and removing it would cause breakages. I am going to clean them up, but you can choose to wait for my cleanup to complete (which delays this PR) or keep the macro for now.

No problem. I've modified the first commit in this PR so that it no longer removes the macro definitions.

PiperOrigin-RevId: 308625388

The ACTION_P generated code introduces -Wdeprecated warning as it provides its own assignment operator implementation but no copy assignment constructor. The issue seem to be fixed in google/googletest#2815 but this is not yet included into latest gtest release.

They were all removed in google#2815, but it looks like this one got added from a Google export which missed the update. See google#2815 for reasons why removing this is desirable.

See google/googletest#2815 for the reasons why this is desirable. Some things are in different places, but most of the same lines exist, so apply the same change to them. Change-Id: I054d2cd00aec96c9e1562f35eeda05cb89ed049e Signed-off-by: Brian Silverman <bsilver16834@gmail.com>

Cherry-pick google/googletest#2815 to fix the -Wdeprecated-copy warnings of recent versions of Clang (13 or later). The patch was suggested by Mark Wachsler. Change-Id: I0cc88543e7bc68af6292ddc3773dad38c459cc72

Update the pinned revision for googletest to pick up google/googletest#2815 which should allow us to repair the macOS builds.

Update the pinned revision for googletest to pick up google/googletest#2815 which should allow us to repair the macOS builds. Add a workaround for dependency tracking to try to avoid a build race.

Update the pinned revision for googletest to pick up google/googletest#2815 which should allow us to repair the macOS builds. Add dependency tracking to try to avoid a build race with capstone and libbloaty.

Cherry-pick google/googletest#2815 to fix the -Wdeprecated-copy warnings of recent versions of Clang (13 or later). The patch was suggested by Mark Wachsler. Change-Id: I0cc88543e7bc68af6292ddc3773dad38c459cc72

googlebot added the cla: yes label Apr 16, 2020

zhangxy988 self-assigned this Apr 17, 2020

zhangxy988 requested a review from mbxx April 17, 2020 16:08

Quuxplusone mentioned this pull request Apr 17, 2020

Fix Clang's -Wdeprecated-copy warnings in C++20 #2758

Closed

zhangxy988 suggested changes Apr 21, 2020

View reviewed changes

zhangxy988 reviewed Apr 22, 2020

View reviewed changes

Quuxplusone force-pushed the simple branch from c458bdb to d6b98fd Compare April 22, 2020 15:12

Quuxplusone added 5 commits April 23, 2020 22:22

Fix a -Wdeprecated warning.

11b3cec

gmock-spec-builders.h:503:3: error: definition of implicit copy constructor for 'Expectation' is deprecated because it has a user-declared destructor [-Werror,-Wdeprecated] ~Expectation(); ^

Fix a -Wdeprecated warning.

01c0ff5

googletest-port-test.cc:97:11: error: definition of implicit copy constructor for 'Base' is deprecated because it has a user-declared destructor [-Werror,-Wdeprecated] virtual ~Base() {} ^

Add -Wdeprecated to the build configuration.

1b066f4

Quuxplusone force-pushed the simple branch from d6b98fd to 1b066f4 Compare April 24, 2020 02:22

rogeeff added a commit that referenced this pull request May 1, 2020

Merge pull request #2815 from Quuxplusone:simple

a3f0602

PiperOrigin-RevId: 308625388

rogeeff merged commit ef25d27 into google:master May 1, 2020

kentanabe mentioned this pull request Jun 25, 2020

Compile error on Ubuntu 20.04 kentanabe/intel-caffe#1

Closed

dcrc2 mentioned this pull request Feb 22, 2021

Compilation fails due to deprecation and -Werror microsoft/ADBench#203

Open

bsilver8192 mentioned this pull request Jan 6, 2022

Remove another GTEST_DISALLOW_ASSIGN_ that crept in #3725

Merged

frc971-automation pushed a commit to frc971/971-Robot-Code that referenced this pull request Jan 9, 2022

Merge "Backport google/googletest#2815 into Ceres's internal copy"

f07263e

Kumataro mentioned this pull request May 1, 2022

build warnings with latest compiler opencv/opencv#21911

Closed

4 tasks

namazso mentioned this pull request Jul 18, 2022

Fix instances of -Wdeprecated-copy #3427

Open

namazso mentioned this pull request Jul 18, 2022

Deprecated implicit copy assignment/constructor #2224

Open

codyjack5 mentioned this pull request Oct 28, 2022

GTest fails with deprecated copy warning on newer compilers (clang-14) oneapi-src/oneVPL-cpu#40

Closed

compnerd added a commit to compnerd/bloaty that referenced this pull request Nov 7, 2022

third_party: update googletest to current main

ef8f234

Update the pinned revision for googletest to pick up google/googletest#2815 which should allow us to repair the macOS builds.

compnerd mentioned this pull request Nov 7, 2022

third_party: update googletest to current main google/bloaty#326

Merged

tmm1 mentioned this pull request Jan 27, 2023

[googletest] bump from v1.10 to v1.13 to fix compile issues xbmc/inputstream.adaptive#1106

Closed

alalek mentioned this pull request Jan 31, 2023

Supress tones of -Wdeprecated-copy warnings opencv/opencv#23203

Merged

6 tasks

JvE-Lely mentioned this pull request Feb 27, 2023

Current Gmock generates a lot of warning on Ubuntu 22.04 with Clang ament/googletest#23

Closed

CyanoKobalamyne mentioned this pull request Feb 7, 2024

Update boolector to most recent version (3.2.3 plus extra) stanford-centaur/smt-switch#344

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix all -Wdeprecated warnings; eliminate GTEST_DISALLOW_ASSIGN_ #2815

Fix all -Wdeprecated warnings; eliminate GTEST_DISALLOW_ASSIGN_ #2815

Quuxplusone commented Apr 16, 2020

zhangxy988 left a comment

Quuxplusone commented Apr 21, 2020

zhangxy988 left a comment

zhangxy988 left a comment

zhangxy988 commented Apr 24, 2020

Quuxplusone commented Apr 24, 2020

Fix all -Wdeprecated warnings; eliminate GTEST_DISALLOW_ASSIGN_ #2815

Fix all -Wdeprecated warnings; eliminate GTEST_DISALLOW_ASSIGN_ #2815

Conversation

Quuxplusone commented Apr 16, 2020

zhangxy988 left a comment

Choose a reason for hiding this comment

Quuxplusone commented Apr 21, 2020

zhangxy988 left a comment

Choose a reason for hiding this comment

zhangxy988 left a comment

Choose a reason for hiding this comment

zhangxy988 commented Apr 24, 2020

Quuxplusone commented Apr 24, 2020