This is an archive of the discontinued LLVM Phabricator instance.

[Diagnostics] Restore -Wdeprecated warning when user-declared copy assignment operator is defined as deleted (PR45634)
ClosedPublic

Authored by xbolva00 on May 11 2020, 6:52 AM.

Details

Summary

Solves https://bugs.llvm.org/show_bug.cgi?id=45634
Be more agressive than GCC with -Wdeprecated-copy. Also provide -W(no-)deprecated-copy-user-provided-copy/dtor options to on/off this behaviour.

Diff Detail

Event Timeline

xbolva00 created this revision.May 11 2020, 6:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 11 2020, 6:52 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
Quuxplusone added inline comments.
clang/include/clang/Basic/DiagnosticGroups.td
158

If we're going to provide these options at all, I think it would be more grammatical to call them -Wdeprecated-copy-with-deleted-copy and -Wdeprecated-copy-with-deleted-dtor.

The existing code is already confusing by talking about a "copy dtor" as if that's a thing; I think it makes it even worse to talk about "deprecated copy user provided," since the actually deprecated thing is the implicitly generated copy.

I get that we're trying to be terse, and also somewhat hierarchical in putting all the -Wdeprecated-copy-related warnings under -Wdeprecated-copy-foo-bar; but the current names are too far into the ungrammatical realm for me personally.

clang/include/clang/Basic/DiagnosticSemaKinds.td
568

This is the first time I've ever seen this idiom. As a person who often greps the wording of an error message to find out where it comes from, I would very strongly prefer to see the actual string literal here. But if this is established practice, then okay.

clang/lib/Sema/SemaDeclCXX.cpp
13864

I wonder if this would be easier to read as

if (UserDeclaredOperation) {
    bool UDOIsUserProvided = UserDeclaredOperation->isUserProvided();
    bool UDOIsDestructor = isa<CXXDestructorDecl>(UserDeclaredOperation);
    bool IsCopyAssignment = !isa<CXXConstructorDecl>(CopyOp);
    unsigned DiagID =
        ( UDOIsUserProvided &&  UDOIsDestructor) ? diag::warn_deprecated_copy_dtor_operation_user_provided :
        ( UDOIsUserProvided && !UDOIsDestructor) ? diag::warn_deprecated_copy_operation_user_provided :
        (!UDOIsUserProvided &&  UDOIsDestructor) ? diag::warn_deprecated_copy_dtor_operation :
        diag::warn_deprecated_copy_operation;
    S.Diag(UserDeclaredOperation->getLocation(), DiagID)
        << RD << /*copy assignment*/ IsCopyAssignment;
clang/test/SemaCXX/deprecated-copy.cpp
7

I'm fairly confident this should just be two different test files. Also, if a test file has an un-ifdeffed // expected-no-diagnostics plus an un-ifdeffed // expected-note ..., which one wins?

xbolva00 marked 4 inline comments as done.May 11 2020, 12:18 PM

Thanks for initial comments.

clang/include/clang/Basic/DiagnosticGroups.td
158

Yeah, better names are welcome :)

clang/include/clang/Basic/DiagnosticSemaKinds.td
568

I think this is established way how to duplicated warning text strings.

clang/lib/Sema/SemaDeclCXX.cpp
13864

I have been thinking about similar form, so +1.

clang/test/SemaCXX/deprecated-copy.cpp
7

ifdef is here

and ifNdef is below :)

xbolva00 marked an inline comment as done.May 11 2020, 12:19 PM
xbolva00 added inline comments.
clang/test/SemaCXX/deprecated-copy.cpp
7

and DEPRECATED_COPY_DTOR is in own "code block"

clang/test/SemaCXX/deprecated-copy.cpp
7

Ah, you're right, I had missed that line 18 was still controlled by the #ifdef DEPRECATED_COPY_DTOR condition.
I still think this should be three(!) different files. Line 2 doesn't even look right to me: shouldn't it be -Wdeprecated-copy -Wno-deprecated-copy-user-provided?

I just did a git grep 'RUN:' | grep '[-]Wno-' | grep -v '[-]W[^n]' and found a massive number of tests that put -Wno-foo on the command line without putting -Wbar anywhere on the same line; I suspect many of these are bugs.

xbolva00 updated this revision to Diff 280692.Jul 25 2020, 12:59 PM

Updated. Thanks for review.

xbolva00 updated this revision to Diff 280693.Jul 25 2020, 1:15 PM

More tests

xbolva00 marked an inline comment as done.Jul 25 2020, 1:16 PM
xbolva00 updated this revision to Diff 280694.Jul 25 2020, 1:26 PM
clang/include/clang/Basic/DiagnosticGroups.td
158

Do the current names match existing practice in GCC or anything?
I continue to opine that these options are poorly named. My best suggestion is
deprecated-copy, deprecated-copy-with-dtor, deprecated-copy-with-deleted-copy, deprecated-copy-with-deleted-dtor — operating on the (wrong?) assumption that absolutely the only difference between "user-declared" and "user-provided" corresponds to "user-declared as deleted."

Even if everything else remains the same, the internal identifier warn_deprecated_copy_dtor_operation_user_provided should certainly be warn_deprecated_copy_operation_dtor_user_provided — the phrase that goes together is "copy operation", not "dtor operation".

Your "deprecated-dtor-user-provided.cpp" passes -Wdeprecated-copy-dtor-user-provided, but the message text talks about "user-declared." Shouldn't the spelling of the option reflect the wording of the message text? This isn't important from the POV of someone who's just going to look at the message text and fix their code, but it's important from the POV of someone who's going to use -Wno- and wants to know exactly which bad situations they're ignoring. (Also, the name of the file should match the spelling of the option.)

clang/lib/Sema/SemaDeclCXX.cpp
13877

The /* copy assignment */ comment is probably no longer useful.

clang/test/SemaCXX/deprecated-copy.cpp
7

I've lost track of what I was trying to say here and whether I was right ;) but I assume we can consider this comment thread "resolved"?

clang/test/SemaCXX/deprecated-dtor-user-provided.cpp
6 ↗(On Diff #280694)

Do you have a test case for when an operation is defaulted as deleted?
I actually don't understand Clang's/GCC's current behavior in this area.
https://godbolt.org/z/9h3qqP
Of these three situations, do any of them rely on deprecated behavior?
(And vice versa, what if we delete the copy constructor and try to use the implicit copy-assignment operator?)

clang/test/SemaCXX/deprecated.cpp
108

Nit: I would prefer to see the declarations of d1 and d2 on individual lines, both for style and to prove that d1's declaration triggers no warnings.

xbolva00 updated this revision to Diff 338334.Apr 17 2021, 1:04 PM
xbolva00 marked 5 inline comments as done.

Updated, addressed review feedback.

xbolva00 edited the summary of this revision. (Show Details)Apr 17 2021, 1:04 PM
xbolva00 updated this revision to Diff 338336.Apr 17 2021, 1:09 PM
xbolva00 added a comment.EditedApr 22 2021, 9:20 AM
clang/include/clang/Basic/DiagnosticGroups.td
158

Yeah, deprecated-copy and deprecated-copy-dtor is already defined by GCC.

I think deprecated-copy-user-provided-copy and deprecated-copy-user-provided-dtor could be good solution here, it is also quite easy to follow implementation of the checking logic in Sema with these names.

Quuxplusone accepted this revision.Apr 22 2021, 10:02 AM

The new individual test files are awesome. :)
The name of the -W options still aren't perfect IMHO, but maybe it will never be perfect, and meanwhile everything else looks great.

clang/include/clang/Basic/DiagnosticGroups.td
158

@xbolva00: I suggested -Wdeprecated-copy-with-user-provided-copy; you've got -Wdeprecated-copy-user-provided-copy. Could I persuade you to use the -with- versions?
I suppose where it really matters is -Wdeprecated-copy-dtor (what is a "copy destructor"?), but that's for GCC compatibility, so we can't change it. Right?

clang/test/SemaCXX/deprecated-copy-dtor.cpp
2 ↗(On Diff #338336)

I think it's extremely unfortunate that -Wdeprecated-copy-dtor is not a subset of -Wdeprecated-copy. But again, GCC-compatibility binds our hands here AFAIK. https://godbolt.org/z/3r3ddvTfG

This revision is now accepted and ready to land.Apr 22 2021, 10:02 AM
xbolva00 added inline comments.Apr 22 2021, 10:26 AM
clang/include/clang/Basic/DiagnosticGroups.td
158

I can use with “-with” and create alias for -Wdeprecated-copy-dtor to keep gcc compatibility :)

xbolva00 updated this revision to Diff 339734.Apr 22 2021, 11:22 AM

Use suggested flag names.

This revision was landed with ongoing or failed builds.Apr 22 2021, 11:35 AM
This revision was automatically updated to reflect the committed changes.

Hi,

With this commit I get failures in the following testcases when building check-runtimes on trunk:

Failed Tests (29):
  libc++ :: libcxx/debug/containers/db_sequence_container_iterators.pass.cpp
  libc++ :: libcxx/gdb/gdb_pretty_printer_test.sh.cpp
  libc++ :: std/containers/sequences/vector.bool/assign_copy.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/assign_move.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/copy.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/copy_alloc.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/emplace.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/erase_iter.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/erase_iter_iter.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/insert_iter_initializer_list.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/insert_iter_iter_iter.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/insert_iter_size_value.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/insert_iter_value.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/iterators.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/move.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/move_alloc.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/resize_size.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/resize_size_value.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/shrink_to_fit.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/size.pass.cpp
  libc++ :: std/containers/sequences/vector/vector.cons/deduct.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/left_shift.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/op_eq_eq.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/right_shift.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/to_ullong.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/to_ulong.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.operators/op_and.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.operators/op_not.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.operators/op_or.pass.cpp

They all get -Wdeprecated-copy warnigns and then with -Werror they fail.
Is this something you've seen or considered?

Hi,

With this commit I get failures in the following testcases when building check-runtimes on trunk:

Failed Tests (29):
  libc++ :: libcxx/debug/containers/db_sequence_container_iterators.pass.cpp
  libc++ :: libcxx/gdb/gdb_pretty_printer_test.sh.cpp
  libc++ :: std/containers/sequences/vector.bool/assign_copy.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/assign_move.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/copy.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/copy_alloc.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/emplace.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/erase_iter.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/erase_iter_iter.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/insert_iter_initializer_list.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/insert_iter_iter_iter.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/insert_iter_size_value.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/insert_iter_value.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/iterators.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/move.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/move_alloc.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/resize_size.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/resize_size_value.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/shrink_to_fit.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/size.pass.cpp
  libc++ :: std/containers/sequences/vector/vector.cons/deduct.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/left_shift.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/op_eq_eq.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/right_shift.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/to_ullong.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/to_ulong.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.operators/op_and.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.operators/op_not.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.operators/op_or.pass.cpp

They all get -Wdeprecated-copy warnigns and then with -Werror they fail.
Is this something you've seen or considered?

Can you try this fix?

diff --git a/libcxx/utils/libcxx/test/params.py b/libcxx/utils/libcxx/test/params.py
index ddf277dea246..abf712e78a61 100644
--- a/libcxx/utils/libcxx/test/params.py
+++ b/libcxx/utils/libcxx/test/params.py
@@ -12,6 +12,7 @@ from libcxx.test.features import _isMSVC
 _warningFlags = [
   '-Werror',
   '-Wall',
+  '-Wno-deprecated-copy',
   '-Wextra',
   '-Wshadow',
   '-Wundef',

Can you try this fix?

diff --git a/libcxx/utils/libcxx/test/params.py b/libcxx/utils/libcxx/test/params.py
index ddf277dea246..abf712e78a61 100644
--- a/libcxx/utils/libcxx/test/params.py
+++ b/libcxx/utils/libcxx/test/params.py
@@ -12,6 +12,7 @@ from libcxx.test.features import _isMSVC
 _warningFlags = [
   '-Werror',
   '-Wall',
+  '-Wno-deprecated-copy',
   '-Wextra',
   '-Wshadow',
   '-Wundef',

The above doesn't help but if I add it in the list after -Wextra the warning goes away. Can you submit something like that?
Thanks!

Can you try this fix?

diff --git a/libcxx/utils/libcxx/test/params.py b/libcxx/utils/libcxx/test/params.py
index ddf277dea246..abf712e78a61 100644
--- a/libcxx/utils/libcxx/test/params.py
+++ b/libcxx/utils/libcxx/test/params.py
@@ -12,6 +12,7 @@ from libcxx.test.features import _isMSVC
 _warningFlags = [
   '-Werror',
   '-Wall',
+  '-Wno-deprecated-copy',
   '-Wextra',
   '-Wshadow',
   '-Wundef',

The above doesn't help but if I add it in the list after -Wextra the warning goes away. Can you submit something like that?
Thanks!

Yes, I can. Thanks

This breaks Clang's ability to bootstrap LLVM with -Werror as evidenced by the buildbot failure https://lab.llvm.org/buildbot/#/builders/36/builds/7587.
Please revert or fix the googletest code that trips this warning. Pertinent file:

FAILED: unittests/Support/CMakeFiles/SupportTests.dir/FormatVariadicTest.cpp.o 
/home/buildbots/ppc64le-lld-multistage-test/ppc64le-lld-multistage-test/install/stage1/bin/clang++  -DGTEST_HAS_RTTI=0 -DGTEST_HAS_TR1_TUPLE=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Iunittests/Support -I/home/buildbots/ppc64le-lld-multistage-test/ppc64le-lld-multistage-test/llvm-project/llvm/unittests/Support -Iinclude -I/home/buildbots/ppc64le-lld-multistage-test/ppc64le-lld-multistage-test/llvm-project/llvm/include -I/home/buildbots/ppc64le-lld-multistage-test/ppc64le-lld-multistage-test/llvm-project/llvm/utils/unittest/googletest/include -I/home/buildbots/ppc64le-lld-multistage-test/ppc64le-lld-multistage-test/llvm-project/llvm/utils/unittest/googlemock/include -fPIC -fvisibility-inlines-hidden -Werror -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -fdiagnostics-color -ffunction-sections -fdata-sections -O3 -DNDEBUG    -Wno-variadic-macros -Wno-gnu-zero-variadic-macro-arguments -fno-exceptions -fno-rtti -UNDEBUG -Wno-suggest-override -std=c++14 -MD -MT unittests/Support/CMakeFiles/SupportTests.dir/FormatVariadicTest.cpp.o -MF unittests/Support/CMakeFiles/SupportTests.dir/FormatVariadicTest.cpp.o.d -o unittests/Support/CMakeFiles/SupportTests.dir/FormatVariadicTest.cpp.o -c /home/buildbots/ppc64le-lld-multistage-test/ppc64le-lld-multistage-test/llvm-project/llvm/unittests/Support/FormatVariadicTest.cpp
In file included from /home/buildbots/ppc64le-lld-multistage-test/ppc64le-lld-multistage-test/llvm-project/llvm/unittests/Support/FormatVariadicTest.cpp:12:
In file included from /home/buildbots/ppc64le-lld-multistage-test/ppc64le-lld-multistage-test/llvm-project/llvm/utils/unittest/googletest/include/gtest/gtest.h:62:
In file included from /home/buildbots/ppc64le-lld-multistage-test/ppc64le-lld-multistage-test/llvm-project/llvm/utils/unittest/googletest/include/gtest/gtest-param-test.h:193:
/home/buildbots/ppc64le-lld-multistage-test/ppc64le-lld-multistage-test/llvm-project/llvm/utils/unittest/googletest/include/gtest/internal/gtest-param-util-generated.h:107:8: error: definition of implicit copy constructor for 'ValueArray2<bool, bool>' is deprecated because it has a user-declared copy assignment operator [-Werror,-Wdeprecated-copy]
  void operator=(const ValueArray2& other) = delete;
       ^

This breaks Clang's ability to bootstrap LLVM with -Werror as evidenced by the buildbot failure https://lab.llvm.org/buildbot/#/builders/36/builds/7587.
Please revert or fix the googletest code that trips this warning. Pertinent file:

FAILED: unittests/Support/CMakeFiles/SupportTests.dir/FormatVariadicTest.cpp.o 
/home/buildbots/ppc64le-lld-multistage-test/ppc64le-lld-multistage-test/install/stage1/bin/clang++  -DGTEST_HAS_RTTI=0 -DGTEST_HAS_TR1_TUPLE=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Iunittests/Support -I/home/buildbots/ppc64le-lld-multistage-test/ppc64le-lld-multistage-test/llvm-project/llvm/unittests/Support -Iinclude -I/home/buildbots/ppc64le-lld-multistage-test/ppc64le-lld-multistage-test/llvm-project/llvm/include -I/home/buildbots/ppc64le-lld-multistage-test/ppc64le-lld-multistage-test/llvm-project/llvm/utils/unittest/googletest/include -I/home/buildbots/ppc64le-lld-multistage-test/ppc64le-lld-multistage-test/llvm-project/llvm/utils/unittest/googlemock/include -fPIC -fvisibility-inlines-hidden -Werror -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -fdiagnostics-color -ffunction-sections -fdata-sections -O3 -DNDEBUG    -Wno-variadic-macros -Wno-gnu-zero-variadic-macro-arguments -fno-exceptions -fno-rtti -UNDEBUG -Wno-suggest-override -std=c++14 -MD -MT unittests/Support/CMakeFiles/SupportTests.dir/FormatVariadicTest.cpp.o -MF unittests/Support/CMakeFiles/SupportTests.dir/FormatVariadicTest.cpp.o.d -o unittests/Support/CMakeFiles/SupportTests.dir/FormatVariadicTest.cpp.o -c /home/buildbots/ppc64le-lld-multistage-test/ppc64le-lld-multistage-test/llvm-project/llvm/unittests/Support/FormatVariadicTest.cpp
In file included from /home/buildbots/ppc64le-lld-multistage-test/ppc64le-lld-multistage-test/llvm-project/llvm/unittests/Support/FormatVariadicTest.cpp:12:
In file included from /home/buildbots/ppc64le-lld-multistage-test/ppc64le-lld-multistage-test/llvm-project/llvm/utils/unittest/googletest/include/gtest/gtest.h:62:
In file included from /home/buildbots/ppc64le-lld-multistage-test/ppc64le-lld-multistage-test/llvm-project/llvm/utils/unittest/googletest/include/gtest/gtest-param-test.h:193:
/home/buildbots/ppc64le-lld-multistage-test/ppc64le-lld-multistage-test/llvm-project/llvm/utils/unittest/googletest/include/gtest/internal/gtest-param-util-generated.h:107:8: error: definition of implicit copy constructor for 'ValueArray2<bool, bool>' is deprecated because it has a user-declared copy assignment operator [-Werror,-Wdeprecated-copy]
  void operator=(const ValueArray2& other) = delete;
       ^

I pushed a fix to just disable this warning for googlemock/gtest.

I pushed a fix to just disable this warning for googlemock/gtest.

There are still more of these. I will apply the same fix to utils/unittest/googlemock/include/gmock/gmock.h unless you think there's a problem with doing that.

I pushed a fix to just disable this warning for googlemock/gtest.

There are still more of these. I will apply the same fix to utils/unittest/googlemock/include/gmock/gmock.h unless you think there's a problem with doing that.

Yeah, please go ahead :)

The fixes in https://reviews.llvm.org/D101214 are sufficient to bring the -Werror bootstrap back to green. Please review the patch.

I pushed a fix to just disable this warning for googlemock/gtest.

Unfortunately that fix breaks compiling llvm with clang8. I commented in
https://reviews.llvm.org/rG9658d045926545e62cc3f963fe611d7c5d0c9d98

I'm seeing here something very strange, if the user provided copy assignment operator is provided but is = delete and the copy constructor is default this warning will still trigger. Is this something expected?
I'm referring at this issue from the mozilla code-base.

I'm seeing here something very strange, if the user provided copy assignment operator is provided but is = delete and the copy constructor is default this warning will still trigger. Is this something expected?
I'm referring at this issue from the mozilla code-base.

Yes, that implicit default is deprecated.
If you want your copy constructor to be defaulted, even in the presence of deleted SMFs, you should explicitly =default it. Here's an example: https://godbolt.org/z/GaGn1638E